From 31e6671e65cfda2e884d433a1c3d1ef4b09e882c Mon Sep 17 00:00:00 2001 From: xRangerDE Date: Sun, 3 May 2026 15:43:01 +0200 Subject: [PATCH] harden: download-clip integrity + cancel tracking + decouple editor procs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two server-side fixes for separate clip/queue/editor crosstalk paths. 1. download-clip IPC was unsafe in three ways: - reported success: true on exit code 0 even with empty files (Twitch sometimes returns a manifest with no segments) - passed clipInfo.broadcaster_name straight to path.join, so unicode / spaces / punctuation in display names produced odd directory layouts on Windows - the spawned streamlink process was tracked nowhere, so window close orphaned it Now: sanitize broadcaster_name + title, ensureUniqueFilename so re-downloads do not overwrite, post-download size + integrity check (16 KiB floor + ffprobe via validateDownloadedFileIntegrity), proc tracked in activeClipProcesses and killed on window-all-closed. 2. currentProcess (a single ChildProcess global) was shared between cutter/merger/splitter and downloadVODPart. The real bug: while a queue download was running and the user kicked off a video cut, pressing the queue's "Stop" button iterated activeDownloads (fine) AND called currentProcess.kill() — which by then pointed at the cutter ffmpeg, killing an unrelated cut. Renamed to currentEditorProcess, confined to the editor pipeline. downloadVODPart no longer touches it. The fallback kill calls in remove-from-queue / pause-download / cancel-download are gone — the activeDownloads loop above each was already authoritative. window-all-closed now also kills activeClipProcesses. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.ts | 114 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 78 insertions(+), 36 deletions(-) diff --git a/src/main.ts b/src/main.ts index fce526e..501763b 100644 --- a/src/main.ts +++ b/src/main.ts @@ -530,7 +530,11 @@ let downloadQueue: QueueItem[] = loadQueue(); let queueIdCounter = 0; let lastQueueBroadcastFingerprint = ''; let isDownloading = false; -let currentProcess: ChildProcess | null = null; +// Process handle for the standalone video editor pipeline (cutter / merger / +// splitter). Queue downloads track their own children via activeDownloads, +// and clip downloads via activeClipProcesses. Keeping these separate +// prevents cancel-download from killing an unrelated cutter ffmpeg. +let currentEditorProcess: ChildProcess | null = null; let currentDownloadCancelled = false; let pauseRequested = false; let activeQueueItemId: string | null = null; @@ -2066,7 +2070,7 @@ async function cutVideo( return await new Promise((resolve) => { const proc = spawn(ffmpeg, args, { windowsHide: true }); - currentProcess = proc; + currentEditorProcess = proc; proc.stdout?.on('data', (data) => { const line = data.toString(); @@ -2079,7 +2083,7 @@ async function cutVideo( }); proc.on('close', (code) => { - currentProcess = null; + currentEditorProcess = null; if (code === 0 && fs.existsSync(outputFile)) { const stats = fs.statSync(outputFile); if (stats.size <= 256) { @@ -2094,7 +2098,7 @@ async function cutVideo( }); proc.on('error', () => { - currentProcess = null; + currentEditorProcess = null; resolve(false); }); }); @@ -2208,7 +2212,7 @@ async function mergeVideos( return await new Promise((resolve) => { const proc = spawn(ffmpeg, args, { windowsHide: true }); - currentProcess = proc; + currentEditorProcess = proc; proc.stdout?.on('data', (data) => { const line = data.toString(); @@ -2224,7 +2228,7 @@ async function mergeVideos( }); proc.on('close', (code) => { - currentProcess = null; + currentEditorProcess = null; const success = code === 0 && fs.existsSync(outputFile); if (success) { onProgress(100); @@ -2233,7 +2237,7 @@ async function mergeVideos( }); proc.on('error', () => { - currentProcess = null; + currentEditorProcess = null; resolve(false); }); }); @@ -2303,15 +2307,15 @@ async function splitMergedFile( const success = await new Promise((resolve) => { const proc = spawn(ffmpeg, args, { windowsHide: true }); - currentProcess = proc; + currentEditorProcess = proc; proc.on('close', (code) => { - currentProcess = null; + currentEditorProcess = null; resolve(code === 0 && fs.existsSync(outputFile)); }); proc.on('error', () => { - currentProcess = null; + currentEditorProcess = null; resolve(false); }); }); @@ -2358,9 +2362,9 @@ function downloadVODPart( appendDebugLog('download-part-start', { itemId, command: streamlinkCmd.command, filename, args }); const proc = spawn(streamlinkCmd.command, args, { windowsHide: true }); - currentProcess = proc; // Register in per-item tracking map for parallel downloads + // (no longer mirrored on a global — currentEditorProcess is editor-only) const itemTracking = { process: proc, cancelled: false, startTime: Date.now(), bytes: 0 }; activeDownloads.set(itemId, itemTracking); @@ -2451,7 +2455,6 @@ function downloadVODPart( proc.on('close', async (code) => { clearInterval(progressInterval); - currentProcess = null; activeDownloads.delete(itemId); if (currentDownloadCancelled || cancelledItemIds.has(itemId)) { @@ -2496,7 +2499,6 @@ function downloadVODPart( proc.on('error', (err) => { clearInterval(progressInterval); console.error('Process error:', err); - currentProcess = null; activeDownloads.delete(itemId); const rawError = String(err); const errorMessage = rawError.includes('ENOENT') @@ -3582,17 +3584,12 @@ ipcMain.handle('remove-from-queue', (_, id: string) => { const wasActiveItem = activeQueueItemId === id || activeDownloads.has(id); if (wasActiveItem) { - // Cancel via per-item tracking cancelledItemIds.add(id); const tracking = activeDownloads.get(id); if (tracking?.process) { tracking.process.kill(); } - // Also set global for backwards compat currentDownloadCancelled = true; - if (currentProcess) { - currentProcess.kill(); - } activeDownloads.delete(id); activeQueueItemId = null; runtimeMetrics.activeItemId = null; @@ -3761,16 +3758,14 @@ ipcMain.handle('pause-download', () => { pauseRequested = true; currentDownloadCancelled = true; - // Kill all active download processes + // Kill queue downloads only — cutter/merger/splitter use currentEditorProcess + // and aren't affected by pause-download. for (const [id, tracking] of activeDownloads) { cancelledItemIds.add(id); if (tracking.process) { tracking.process.kill(); } } - if (currentProcess) { - currentProcess.kill(); - } return true; }); @@ -3778,16 +3773,13 @@ ipcMain.handle('cancel-download', () => { isDownloading = false; pauseRequested = false; currentDownloadCancelled = true; - // Kill all active download processes + // Kill queue downloads only — see pause-download note above. for (const [id, tracking] of activeDownloads) { cancelledItemIds.add(id); if (tracking.process) { tracking.process.kill(); } } - if (currentProcess) { - currentProcess.kill(); - } return true; }); @@ -3858,6 +3850,11 @@ ipcMain.handle('open-external', async (_, url: string) => { await shell.openExternal(url); }); +// Tracks active standalone clip downloads so cancel-download / window-all-closed +// can kill them. Separate from activeDownloads (queue) because clip downloads +// don't go through the queue scheduler. +const activeClipProcesses = new Map(); + ipcMain.handle('download-clip', async (_, clipUrl: string) => { let clipId = ''; const match1 = clipUrl.match(/clips\.twitch\.tv\/([A-Za-z0-9_-]+)/); @@ -3870,7 +3867,14 @@ ipcMain.handle('download-clip', async (_, clipUrl: string) => { const clipInfo = await getClipInfo(clipId); if (!clipInfo) return { success: false, error: 'Clip nicht gefunden' }; - const folder = path.join(config.download_path, 'Clips', clipInfo.broadcaster_name); + // Sanitize broadcaster_name for path safety — Twitch returns the display + // name which can contain unicode, spaces, or punctuation that breaks + // path joining on some Windows configurations. + const safeBroadcaster = sanitizeFilenamePart( + typeof clipInfo.broadcaster_name === 'string' ? clipInfo.broadcaster_name : '', + 'unknown' + ); + const folder = path.join(config.download_path, 'Clips', safeBroadcaster); fs.mkdirSync(folder, { recursive: true }); const clipDiskCheck = ensureDiskSpace(folder, 128 * 1024 * 1024, 'Clip-Download'); @@ -3878,10 +3882,14 @@ ipcMain.handle('download-clip', async (_, clipUrl: string) => { return { success: false, error: clipDiskCheck.error || 'Zu wenig Speicherplatz.' }; } - const safeTitle = clipInfo.title.replace(/[^a-zA-Z0-9_\- ]/g, '').substring(0, 50); - const filename = path.join(folder, `${safeTitle}.mp4`); + const rawTitle = typeof clipInfo.title === 'string' ? clipInfo.title : ''; + const safeTitle = (rawTitle.replace(/[^a-zA-Z0-9_\- ]/g, '').trim().substring(0, 50)) || 'clip'; + // Use ensureUniqueFilename so retrying a clip with the same title doesn't + // overwrite the previous download. itemId is the clipId — if the user + // cancels via cancel-download, that's the handle. + const filename = ensureUniqueFilename(path.join(folder, `${safeTitle}.mp4`), clipId); - return new Promise((resolve) => { + return new Promise<{ success: boolean; error?: string; filename?: string }>((resolve) => { const streamlinkCmd = getStreamlinkCommand(); const proc = spawn(streamlinkCmd.command, [ ...streamlinkCmd.prefixArgs, @@ -3891,15 +3899,45 @@ ipcMain.handle('download-clip', async (_, clipUrl: string) => { '--force' ], { windowsHide: true }); + activeClipProcesses.set(clipId, proc); + appendDebugLog('clip-download-start', { clipId, broadcaster: safeBroadcaster, filename }); + proc.on('close', (code) => { - if (code === 0 && fs.existsSync(filename)) { - resolve({ success: true, filename }); - } else { + activeClipProcesses.delete(clipId); + releaseClaimedFilenamesForItem(clipId); + + if (code !== 0 || !fs.existsSync(filename)) { + appendDebugLog('clip-download-failed', { clipId, code }); resolve({ success: false, error: `Download fehlgeschlagen (Exit-Code ${code ?? -1})` }); + return; } + + // Integrity: clips are short but should still be at least a few KB + // and parse as a video stream via ffprobe. Empty/zero-byte files + // were previously reported as "success" because exit code was 0. + const stats = fs.statSync(filename); + if (stats.size < 16 * 1024) { + try { fs.unlinkSync(filename); } catch { } + appendDebugLog('clip-download-too-small', { clipId, bytes: stats.size }); + resolve({ success: false, error: `Clip-Datei zu klein (${stats.size} Bytes) — Twitch hat den Stream evtl. nicht ausgeliefert.` }); + return; + } + + const integrity = validateDownloadedFileIntegrity(filename, null); + if (!integrity.success) { + try { fs.unlinkSync(filename); } catch { } + appendDebugLog('clip-download-integrity-failed', { clipId, error: integrity.error }); + resolve({ success: false, error: integrity.error || 'Integritaetspruefung fehlgeschlagen.' }); + return; + } + + appendDebugLog('clip-download-success', { clipId, bytes: stats.size, filename }); + resolve({ success: true, filename }); }); proc.on('error', () => { + activeClipProcesses.delete(clipId); + releaseClaimedFilenamesForItem(clipId); resolve({ success: false, error: 'Streamlink nicht gefunden' }); }); }); @@ -4018,14 +4056,18 @@ app.on('window-all-closed', () => { stopDebugLogFlushTimer(true); stopAutoUpdatePolling(); - // Kill all active download processes + // Kill all active children: queue downloads, standalone clip downloads, + // and any in-flight cutter/merger/splitter ffmpeg. for (const [, tracking] of activeDownloads) { if (tracking.process) { tracking.process.kill(); } } - if (currentProcess) { - currentProcess.kill(); + for (const [, proc] of activeClipProcesses) { + try { proc.kill(); } catch { } + } + if (currentEditorProcess) { + currentEditorProcess.kill(); } saveConfig(config); flushQueueSave();