From 099e211f5af788152773e6196a5c98a4f7067799 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Sun, 6 Sep 2026 14:03:49 +0700 Subject: [PATCH] fix(conversion): reserve FLAC outputs without overwriting files Reuse the conversion output lifecycle for M4A conversion and native FLAC suffix correction. Reserve destination names atomically, reject empty output, and clean partial files while preserving the input on failure. Add regressions for existing outputs, concurrent conversions, staging, and cleanup. --- .../download_queue_provider_finalization.dart | 10 +- .../download_queue_provider_single_item.dart | 17 +-- lib/services/ffmpeg_service.dart | 101 +++++++++---- test/ffmpeg_conversion_output_test.dart | 139 ++++++++++++++++++ 4 files changed, 219 insertions(+), 48 deletions(-) create mode 100644 test/ffmpeg_conversion_output_test.dart diff --git a/lib/providers/download_queue_provider_finalization.dart b/lib/providers/download_queue_provider_finalization.dart index 0db4eb10..fda710e8 100644 --- a/lib/providers/download_queue_provider_finalization.dart +++ b/lib/providers/download_queue_provider_finalization.dart @@ -1276,15 +1276,7 @@ extension _DownloadQueueFinalization on DownloadQueueNotifier { return filePath; } if (isAlreadyNativeFlac) { - var flacPath = filePath; - if (!filePath.toLowerCase().endsWith('.flac')) { - final renamedPath = filePath.replaceAll(RegExp(r'\.[^.]+$'), '.flac'); - final targetPath = renamedPath == filePath - ? '$filePath.flac' - : renamedPath; - await File(filePath).rename(targetPath); - flacPath = targetPath; - } + final flacPath = await FFmpegService.ensureNativeFlacExtension(filePath); await embedFlacMetadata(flacPath); markFinalOutputAsFlac(); return flacPath; diff --git a/lib/providers/download_queue_provider_single_item.dart b/lib/providers/download_queue_provider_single_item.dart index 44ddf5ed..db25a6df 100644 --- a/lib/providers/download_queue_provider_single_item.dart +++ b/lib/providers/download_queue_provider_single_item.dart @@ -1459,19 +1459,10 @@ class _DownloadRun { 'Native FLAC payload detected; ensuring .flac ' 'extension and embedding metadata.', ); - var flacPath = currentFilePath; - if (!currentFilePath.toLowerCase().endsWith('.flac')) { - final renamedPath = currentFilePath.replaceAll( - RegExp(r'\.[^.]+$'), - '.flac', - ); - final targetPath = renamedPath == currentFilePath - ? '$currentFilePath.flac' - : renamedPath; - await File(currentFilePath).rename(targetPath); - flacPath = targetPath; - filePath = targetPath; - } + final flacPath = await FFmpegService.ensureNativeFlacExtension( + currentFilePath, + ); + filePath = flacPath; await _embedFinalMetadata(flacPath, format: 'flac'); _markFinalOutputAsFlac(); diff --git a/lib/services/ffmpeg_service.dart b/lib/services/ffmpeg_service.dart index ed838b55..890d3006 100644 --- a/lib/services/ffmpeg_service.dart +++ b/lib/services/ffmpeg_service.dart @@ -103,17 +103,27 @@ class FFmpegService { : firstPath == secondPath; } - static Future _uniqueConversionPath(String requestedPath) async { - if (!await File(requestedPath).exists()) return requestedPath; + // Reserve names atomically: concurrent conversions may choose the same + // sibling basename before either FFmpeg process has created its output. + static Future _reserveConversionPath(String requestedPath) async { final file = File(requestedPath); final fileName = file.uri.pathSegments.last; final dotIndex = fileName.lastIndexOf('.'); final baseName = dotIndex > 0 ? fileName.substring(0, dotIndex) : fileName; final extension = dotIndex > 0 ? fileName.substring(dotIndex) : ''; - for (var index = 2; ; index++) { - final candidate = - '${file.parent.path}${Platform.pathSeparator}$baseName ($index)$extension'; - if (!await File(candidate).exists()) return candidate; + for (var index = 1; ; index++) { + final candidate = index == 1 + ? requestedPath + : '${file.parent.path}${Platform.pathSeparator}$baseName ($index)$extension'; + try { + await File(candidate).create(exclusive: true); + return candidate; + } on FileSystemException { + if (await FileSystemEntity.type(candidate, followLinks: false) == + FileSystemEntityType.notFound) { + rethrow; + } + } } } @@ -133,13 +143,14 @@ class FFmpegService { if (_sameLocalPath(requestedPath, inputPath) && deleteOriginal) { final token = DateTime.now().microsecondsSinceEpoch; return _ConversionOutputPlan( - workingPath: - '${inputFile.parent.path}${Platform.pathSeparator}.$baseName.spotiflac-$token$normalizedExt', + workingPath: await _reserveConversionPath( + '${inputFile.parent.path}${Platform.pathSeparator}.$baseName.spotiflac-$token$normalizedExt', + ), finalPath: inputPath, ); } - final finalPath = await _uniqueConversionPath(requestedPath); + final finalPath = await _reserveConversionPath(requestedPath); return _ConversionOutputPlan(workingPath: finalPath, finalPath: finalPath); } @@ -159,8 +170,10 @@ class FFmpegService { required String inputPath, required bool deleteOriginal, }) async { - if (!await File(plan.workingPath).exists()) { - _log.e('Converted output is missing: ${plan.workingPath}'); + final workingFile = File(plan.workingPath); + if (!await workingFile.exists() || await workingFile.length() == 0) { + _log.e('Converted output is missing or empty: ${plan.workingPath}'); + await _cleanupConversionOutput(plan); return null; } @@ -665,25 +678,61 @@ class FFmpegService { ..add('aresample=${options.join(':')}'); } - static Future convertM4aToFlac(String inputPath) async { - final outputPath = _buildOutputPath(inputPath, '.flac'); - - final command = - '-v error -xerror -i "$inputPath" -c:a flac -compression_level 8 "$outputPath" -y'; - - final result = await _execute(command); - - if (result.success) { - try { - await File(inputPath).delete(); - } catch (_) {} - return outputPath; + static Future convertM4aToFlac( + String inputPath, { + @visibleForTesting Future Function(List)? execute, + }) async { + final plan = await _conversionOutputPlan( + inputPath, + '.flac', + deleteOriginal: true, + ); + try { + final result = await (execute ?? _executeWithArguments)([ + '-v', + 'error', + '-xerror', + '-i', + inputPath, + '-c:a', + 'flac', + '-compression_level', + '8', + plan.workingPath, + '-y', + ]); + if (result.success) { + return await _finalizeConversionOutput( + plan: plan, + inputPath: inputPath, + deleteOriginal: true, + ); + } + _log.e('M4A to FLAC conversion failed: ${result.output}'); + } catch (e) { + _log.e('M4A to FLAC conversion failed: $e'); } - - _log.e('M4A to FLAC conversion failed: ${result.output}'); + await _cleanupConversionOutput(plan); return null; } + /// Corrects a native FLAC payload's suffix without replacing a sibling file. + static Future ensureNativeFlacExtension(String inputPath) async { + if (inputPath.toLowerCase().endsWith('.flac')) return inputPath; + final plan = await _conversionOutputPlan( + inputPath, + '.flac', + deleteOriginal: true, + ); + try { + await File(inputPath).rename(plan.finalPath); + return plan.finalPath; + } catch (_) { + await _cleanupConversionOutput(plan); + rethrow; + } + } + static Future convertM4aToLossy( String inputPath, { required String format, diff --git a/test/ffmpeg_conversion_output_test.dart b/test/ffmpeg_conversion_output_test.dart new file mode 100644 index 00000000..d214531f --- /dev/null +++ b/test/ffmpeg_conversion_output_test.dart @@ -0,0 +1,139 @@ +import 'dart:async'; +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:spotiflac_android/services/ffmpeg_service.dart'; + +void main() { + late Directory directory; + setUp(() async { + directory = await Directory.systemTemp.createTemp('conversion-output-'); + }); + tearDown(() async => directory.delete(recursive: true)); + + Future input(String name) => + File('${directory.path}/$name').writeAsString('source'); + + Future succeed(List arguments) async { + await File(arguments[arguments.length - 2]).writeAsString('converted'); + return FFmpegResult(success: true, returnCode: 0, output: ''); + } + + test( + 'conversion preserves existing sibling and handles quoted paths', + () async { + final source = await input('Song "live".m4a'); + final sibling = await File( + '${directory.path}/Song "live".flac', + ).writeAsString('existing'); + final result = await FFmpegService.convertM4aToFlac( + source.path, + execute: (arguments) async { + expect(arguments[arguments.indexOf('-i') + 1], source.path); + expect(await source.exists(), isTrue); + return succeed(arguments); + }, + ); + expect(result, '${directory.path}/Song "live" (2).flac'); + expect(await sibling.readAsString(), 'existing'); + expect(await File(result!).readAsString(), 'converted'); + expect(await source.exists(), isFalse); + }, + ); + + test('failed conversion removes partial output and retains source', () async { + final source = await input('Song.m4a'); + final result = await FFmpegService.convertM4aToFlac( + source.path, + execute: (arguments) async { + await File(arguments[arguments.length - 2]).writeAsString('partial'); + return FFmpegResult(success: false, returnCode: 1, output: 'failure'); + }, + ); + expect(result, isNull); + expect(await source.readAsString(), 'source'); + expect(await directory.list().length, 1); + }); + + test( + 'exceptions and empty successful outputs preserve the original', + () async { + final source = await input('Song.m4a'); + for (final execute in Function(List)>[ + (_) async => throw StateError('execution failed'), + (_) async => FFmpegResult(success: true, returnCode: 0, output: ''), + ]) { + expect( + await FFmpegService.convertM4aToFlac(source.path, execute: execute), + isNull, + ); + expect(await source.readAsString(), 'source'); + expect(await directory.list().length, 1); + } + }, + ); + + test('same-suffix conversion stages output until it is complete', () async { + final source = await input('Song.flac'); + final result = await FFmpegService.convertM4aToFlac( + source.path, + execute: (arguments) async { + expect(arguments[arguments.length - 2], isNot(source.path)); + expect(await source.readAsString(), 'source'); + return succeed(arguments); + }, + ); + expect(result, source.path); + expect(await source.readAsString(), 'converted'); + expect(await directory.list().length, 1); + }); + + test('concurrent conversions reserve distinct output paths', () async { + final first = await input('Song.m4a'); + final second = await input('Song.mp4'); + final bothStarted = Completer(); + final outputPaths = {}; + Future execute(List arguments) async { + outputPaths.add(arguments[arguments.length - 2]); + if (outputPaths.length == 2) bothStarted.complete(); + await bothStarted.future.timeout(const Duration(seconds: 3)); + return succeed(arguments); + } + + final results = await Future.wait([ + FFmpegService.convertM4aToFlac(first.path, execute: execute), + FFmpegService.convertM4aToFlac(second.path, execute: execute), + ]); + expect(results.toSet(), hasLength(2)); + expect(results, everyElement(isNotNull)); + expect(await directory.list().length, 2); + }); + + test( + 'native FLAC rename preserves sibling files and adds missing suffix', + () async { + final source = await input('Song.m4a'); + final sibling = await File( + '${directory.path}/Song.flac', + ).writeAsString('existing'); + final result = await FFmpegService.ensureNativeFlacExtension(source.path); + expect(result, '${directory.path}/Song (2).flac'); + expect(await sibling.readAsString(), 'existing'); + expect(await File(result).readAsString(), 'source'); + expect(await FFmpegService.ensureNativeFlacExtension(result), result); + final noSuffix = await input('Other'); + expect( + await FFmpegService.ensureNativeFlacExtension(noSuffix.path), + '${noSuffix.path}.flac', + ); + }, + ); + + test('failed native rename releases its reserved destination', () async { + await expectLater( + FFmpegService.ensureNativeFlacExtension('${directory.path}/missing.m4a'), + throwsA(isA()), + ); + expect(await directory.list().length, 0); + }); +}