From cd313e1bfd4d8a7ce6af8233110b6544abbd9bf1 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Tue, 6 Oct 2026 21:59:53 +0700 Subject: [PATCH] fix(history): serialize mutations and preserve newer file metadata --- .../download_history.dart} | 2 +- lib/providers/download_history_provider.dart | 257 ++++++++----- ...download_history_provider_maintenance.dart | 27 +- lib/services/conversion_library_service.dart | 2 +- lib/services/history_database.dart | 52 ++- lib/services/history_maintenance.dart | 71 ++++ test/history_maintenance_test.dart | 364 +++++++++++++++++- 7 files changed, 642 insertions(+), 133 deletions(-) rename lib/{providers/download_history_models.dart => models/download_history.dart} (99%) diff --git a/lib/providers/download_history_models.dart b/lib/models/download_history.dart similarity index 99% rename from lib/providers/download_history_models.dart rename to lib/models/download_history.dart index 30582b6d..20f99428 100644 --- a/lib/providers/download_history_models.dart +++ b/lib/models/download_history.dart @@ -1,4 +1,4 @@ -part of 'download_history_provider.dart'; +import 'package:spotiflac_android/utils/string_utils.dart'; class DownloadHistoryItem { final String id; diff --git a/lib/providers/download_history_provider.dart b/lib/providers/download_history_provider.dart index 9272d39f..2d6c4c52 100644 --- a/lib/providers/download_history_provider.dart +++ b/lib/providers/download_history_provider.dart @@ -3,9 +3,11 @@ import 'dart:math'; import 'dart:io'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:shared_preferences/shared_preferences.dart'; +import 'package:spotiflac_android/models/download_history.dart'; import 'package:spotiflac_android/models/track.dart'; import 'package:spotiflac_android/services/platform_bridge.dart'; import 'package:spotiflac_android/services/history_database.dart'; +import 'package:spotiflac_android/services/history_maintenance.dart'; import 'package:spotiflac_android/utils/logger.dart' hide log; import 'package:spotiflac_android/utils/file_access.dart'; import 'package:spotiflac_android/utils/string_utils.dart'; @@ -14,7 +16,8 @@ import 'package:spotiflac_android/utils/int_utils.dart'; import 'package:spotiflac_android/utils/lyrics_metadata_helper.dart'; import 'package:spotiflac_android/utils/path_match_keys.dart'; -part 'download_history_models.dart'; +export 'package:spotiflac_android/models/download_history.dart'; + part 'download_history_provider_maintenance.dart'; final _historyLog = AppLogger('DownloadHistory'); @@ -325,6 +328,25 @@ class DownloadHistoryNotifier extends Notifier { return pending; } + /// Background probes return sparse changes; this owner reconciles them + /// against the current stored record before publishing their result. + Future applyMaintenanceUpdates( + Iterable updates, + ) => _enqueueHistoryWrite(() async { + final merged = []; + for (final update in updates) { + final json = await _db.getById(update.original.id); + if (json == null) continue; + final item = update.mergeInto(DownloadHistoryItem.fromJson(json)); + if (item != null) merged.add(item); + } + final persistedIds = await _db.updateExistingBatch( + merged.map((item) => item.toJson()).toList(growable: false), + ); + if (!ref.mounted || persistedIds.isEmpty) return; + state = mergeHistoryMaintenanceUpdates(state, merged, persistedIds); + }); + Future _persistHistoryItem( DownloadHistoryItem item, String action, { @@ -369,7 +391,7 @@ class DownloadHistoryNotifier extends Notifier { }); } - Future _bumpIndexNow() async { + Future _bumpIndexNow() => _enqueueHistoryWrite(() async { int? persistedCount; try { persistedCount = await _db.getCount(); @@ -380,7 +402,7 @@ class DownloadHistoryNotifier extends Notifier { totalCount: persistedCount ?? state.totalCount, loadedIndexVersion: state.loadedIndexVersion + 1, ); - } + }); DownloadHistoryItem _putInMemoryTrackVariant(DownloadHistoryItem item) { final isReplacement = state.items.any((existing) => existing.id == item.id); @@ -401,25 +423,7 @@ class DownloadHistoryNotifier extends Notifier { return item; } - void removeFromHistory(String id) { - state = state.copyWith( - items: state.items.where((item) => item.id != id).toList(), - totalCount: state.totalCount > 0 - ? state.totalCount - 1 - : state.totalCount, - lookupItems: state.lookupItems - .where((item) => item.id != id) - .toList(growable: false), - ); - _db - .deleteById(id) - .catchError((Object e) { - _historyLog.e('Failed to delete from database: $e'); - }) - .then((_) { - _bumpHistoryRevision(); - }); - } + Future removeFromHistory(String id) => removeManyFromHistory([id]); Future removePhysicalFiles(Iterable filePaths) { final paths = filePaths.toList(); @@ -431,8 +435,11 @@ class DownloadHistoryNotifier extends Notifier { } Future removeManyFromHistory(Iterable ids) async { + final requestedIds = ids.toList(growable: false); try { - await _removePersistedHistoryItems(ids); + await _enqueueHistoryWrite( + () => _removePersistedHistoryItems(requestedIds), + ); } catch (error, stack) { _historyLog.e('Failed to delete from database: $error', error, stack); } @@ -564,7 +571,7 @@ class DownloadHistoryNotifier extends Notifier { int? lyricsMetadataScanVersion, bool? hasReplayGain, int? replayGainMetadataScanVersion, - }) async { + }) => _enqueueHistoryWrite(() async { final target = await _historyItemForUpdate(id); if (target == null) { _historyLog.w( @@ -614,6 +621,7 @@ class DownloadHistoryNotifier extends Notifier { return; } + await _db.upsert(updated.toJson()); final updatedItems = target.index >= 0 ? ([...state.items]..[target.index] = updated) : state.items; @@ -621,12 +629,11 @@ class DownloadHistoryNotifier extends Notifier { items: updatedItems, lookupItems: _lookupItemsWithUpdates([updated]), ); - await _db.upsert(updated.toJson()); // Swiping through older tracks can backfill several quality records in a // short burst. Coalesce their DB-derived Library refreshes just like // download completion writes instead of rebuilding every view per swipe. _scheduleIndexBump(); - } + }); Future updateMetadataForItem({ required String id, @@ -647,7 +654,7 @@ class DownloadHistoryNotifier extends Notifier { bool? explicit, bool? hasLyrics, int? lyricsMetadataScanVersion, - }) async { + }) => _enqueueHistoryWrite(() async { final target = await _historyItemForUpdate(id); if (target == null) { _historyLog.w('Cannot update metadata for missing history item: $id'); @@ -675,6 +682,7 @@ class DownloadHistoryNotifier extends Notifier { lyricsMetadataScanVersion: lyricsMetadataScanVersion, ); + await _db.upsert(updated.toJson()); final updatedItems = target.index >= 0 ? ([...state.items]..[target.index] = updated) : state.items; @@ -682,9 +690,8 @@ class DownloadHistoryNotifier extends Notifier { items: updatedItems, lookupItems: _lookupItemsWithUpdates([updated]), ); - await _db.upsert(updated.toJson()); _bumpHistoryRevision(); - } + }); static const _audioExtensions = [ '.flac', @@ -880,10 +887,9 @@ class DownloadHistoryNotifier extends Notifier { return; } final deletedSet = deletedIds.toSet(); - final updatedItems = []; - for (final item in state.items) { + DownloadHistoryItem? updateItem(DownloadHistoryItem item) { if (deletedSet.contains(item.id)) { - continue; + return null; } final replacementPath = replacementPaths[item.id]; final replacementFileName = replacementFileNames[item.id]; @@ -894,28 +900,96 @@ class DownloadHistoryNotifier extends Notifier { replacementFileName != item.safFileName) || (replacementRelativeDir != null && replacementRelativeDir != item.safRelativeDir))) { - updatedItems.add( - item.copyWith( - filePath: replacementPath, - safFileName: replacementFileName, - safRelativeDir: replacementRelativeDir, - ), + return item.copyWith( + filePath: replacementPath, + safFileName: replacementFileName, + safRelativeDir: replacementRelativeDir, ); - } else { - updatedItems.add(item); } + return item; } + state = state.copyWith( - items: updatedItems, + items: state.items + .map(updateItem) + .whereType() + .toList(), loadedIndexVersion: state.loadedIndexVersion + 1, - lookupItems: _lookupItemsWithUpdates( - updatedItems, - deletedIds: deletedSet, - ), + lookupItems: state.lookupItems + .map(updateItem) + .whereType() + .toList(growable: false), totalCount: max(0, state.totalCount - deletedSet.length), ); } + Future<({int deleted, int repaired})> _commitInspectedHistoryChanges({ + required List> entries, + required List orphanedIds, + required Map replacementPaths, + required Map replacementFileNames, + required Map replacementRelativeDirs, + }) async { + var outcome = (deleted: 0, repaired: 0); + await _enqueueHistoryWrite(() async { + final byId = {for (final entry in entries) entry['id'] as String: entry}; + final deletions = >[]; + final repairedPaths = {}; + final repairedNames = {}; + final repairedDirs = {}; + for (final id in {...orphanedIds, ...replacementPaths.keys}) { + final entry = byId[id]; + final json = await _db.getById(id); + if (entry == null || json == null) continue; + final current = DownloadHistoryItem.fromJson(json); + final inspectedAt = DateTime.tryParse( + entry['downloaded_at']?.toString() ?? '', + ); + if (current.filePath != entry['file_path'] || + current.storageMode != entry['storage_mode'] || + current.downloadTreeUri != entry['download_tree_uri'] || + (inspectedAt != null && + !current.downloadedAt.isAtSameMomentAs(inspectedAt))) { + continue; + } + final replacement = replacementPaths[id]; + if (replacement == null) { + deletions.add(entry); + continue; + } + final name = current.safFileName == entry['saf_file_name'] + ? replacementFileNames[id] + : null; + final relativeDir = current.safRelativeDir == entry['saf_relative_dir'] + ? replacementRelativeDirs[id] + : null; + final repaired = await _db.updateFilePath( + id, + replacement, + newSafFileName: name, + newSafRelativeDir: relativeDir, + expectedFilePath: entry['file_path'] as String, + expectedDownloadedAt: entry['downloaded_at'] as String?, + ); + if (!repaired) continue; + repairedPaths[id] = replacement; + if (name != null) repairedNames[id] = name; + if (relativeDir != null) repairedDirs[id] = relativeDir; + } + final deletedIds = deletions.isEmpty + ? [] + : await _db.deleteInspectedEntries(deletions); + _applyHistoryPathAndDeletionChanges( + deletedIds: deletedIds, + replacementPaths: repairedPaths, + replacementFileNames: repairedNames, + replacementRelativeDirs: repairedDirs, + ); + outcome = (deleted: deletedIds.length, repaired: repairedPaths.length); + }); + return outcome; + } + Future _cleanupOrphanedDownloadsIncremental({ required int maxItems, required SharedPreferences prefs, @@ -955,22 +1029,13 @@ class DownloadHistoryNotifier extends Notifier { } } for (final replacement in result.replacementPaths.entries) { - await _db.updateFilePath( - replacement.key, - replacement.value, - newSafFileName: result.replacementFileNames[replacement.key], - newSafRelativeDir: result.replacementRelativeDirs[replacement.key], - ); await prefs.remove('$_startupOrphanSuspectPrefix${replacement.key}'); } final confirmedOrphanIds = decision.confirmedIds.toList(growable: false); - final deletedCount = confirmedOrphanIds.isEmpty - ? 0 - : await _db.deleteByIds(confirmedOrphanIds); - - _applyHistoryPathAndDeletionChanges( - deletedIds: confirmedOrphanIds, + final committed = await _commitInspectedHistoryChanges( + entries: entries, + orphanedIds: confirmedOrphanIds, replacementPaths: result.replacementPaths, replacementFileNames: result.replacementFileNames, replacementRelativeDirs: result.replacementRelativeDirs, @@ -985,12 +1050,12 @@ class DownloadHistoryNotifier extends Notifier { await prefs.setInt(_startupOrphanCursorKey, nextCursor); } - if (deletedCount > 0 || result.replacementPaths.isNotEmpty) { + if (committed.deleted > 0 || committed.repaired > 0) { _historyLog.i( - 'Startup orphan cleanup pass: removed=$deletedCount, repaired=${result.replacementPaths.length}, checked=${entries.length}', + 'Startup orphan cleanup pass: removed=${committed.deleted}, repaired=${committed.repaired}, checked=${entries.length}', ); } - return deletedCount; + return committed.deleted; } Future cleanupOrphanedDownloads() async { @@ -999,6 +1064,7 @@ class DownloadHistoryNotifier extends Notifier { final replacementPaths = {}; final replacementFileNames = {}; final replacementRelativeDirs = {}; + final inspectedEntries = >[]; const pageSize = 256; var offset = 0; @@ -1011,6 +1077,7 @@ class DownloadHistoryNotifier extends Notifier { break; } + inspectedEntries.addAll(entries); final result = await _inspectOrphanedEntries(entries); orphanedIds.addAll(result.orphanedIds); replacementPaths.addAll(result.replacementPaths); @@ -1026,75 +1093,51 @@ class DownloadHistoryNotifier extends Notifier { offset += entries.length; } - for (final replacement in replacementPaths.entries) { - await _db.updateFilePath( - replacement.key, - replacement.value, - newSafFileName: replacementFileNames[replacement.key], - newSafRelativeDir: replacementRelativeDirs[replacement.key], - ); - } - if (orphanedIds.isEmpty && replacementPaths.isEmpty) { _historyLog.i('No orphaned entries found'); return 0; } - final deletedCount = orphanedIds.isEmpty - ? 0 - : await _db.deleteByIds(orphanedIds); - _applyHistoryPathAndDeletionChanges( - deletedIds: orphanedIds, + final committed = await _commitInspectedHistoryChanges( + entries: inspectedEntries, + orphanedIds: orphanedIds, replacementPaths: replacementPaths, replacementFileNames: replacementFileNames, replacementRelativeDirs: replacementRelativeDirs, ); _historyLog.i( - 'Cleaned up $deletedCount orphaned entries and repaired ${replacementPaths.length} paths', + 'Cleaned up ${committed.deleted} orphaned entries and repaired ${committed.repaired} paths', ); - return deletedCount; + return committed.deleted; } - void clearHistory() { - state = DownloadHistoryState(loadedIndexVersion: state.loadedIndexVersion); - _db - .clearAll() - .then((_) { - _bumpHistoryRevision(); - }) - .catchError((Object e) { - _historyLog.e('Failed to clear database: $e'); - }); + Future clearHistory() async { + try { + await _enqueueHistoryWrite(() async { + await _db.clearAll(); + state = DownloadHistoryState( + loadedIndexVersion: state.loadedIndexVersion + 1, + ); + }); + } catch (error, stack) { + _historyLog.e('Failed to clear database: $error', error, stack); + } } /// Replaces all download history with [items] (each in the /// [DownloadHistoryItem.toJson] shape) from a restored backup, then reloads /// the in-memory state from storage. - Future restoreFromBackup(List> items) async { - await _db.clearAll(); - if (items.isNotEmpty) { - await _db.upsertBatch(items); - } - await reloadFromStorage(); - } + Future restoreFromBackup(List> items) => + restoreFromBackupStream(Stream.fromIterable(items)); /// Restores a large v2 backup without retaining the complete history in /// Dart memory. SQLite writes are grouped to keep JNI/channel overhead low. - Future restoreFromBackupStream( - Stream> items, - ) async { - await _db.clearAll(); - var batch = >[]; - await for (final item in items) { - batch.add(item); - if (batch.length < 500) continue; - await _db.upsertBatch(batch); - batch = >[]; - } - if (batch.isNotEmpty) await _db.upsertBatch(batch); - await reloadFromStorage(); - } + Future restoreFromBackupStream(Stream> items) => + _enqueueHistoryWrite(() async { + await _db.replaceAll(items); + await _loadFromDatabase(); + }); } final downloadHistoryProvider = diff --git a/lib/providers/download_history_provider_maintenance.dart b/lib/providers/download_history_provider_maintenance.dart index e324eab2..3f102e99 100644 --- a/lib/providers/download_history_provider_maintenance.dart +++ b/lib/providers/download_history_provider_maintenance.dart @@ -4,17 +4,6 @@ part of 'download_history_provider.dart'; /// Startup maintenance: SAF repair, orphan cleanup, and audio-metadata /// backfill that run staggered after the initial history load. extension _HistoryStartupMaintenance on DownloadHistoryNotifier { - Future _commitMaintenanceUpdates(List> updates) => - _enqueueHistoryWrite(() async { - final persistedIds = await _db.updateExistingBatch(updates); - if (!ref.mounted || persistedIds.isEmpty) return; - state = mergeHistoryMaintenanceUpdates( - state, - updates.map(DownloadHistoryItem.fromJson), - persistedIds, - ); - }); - void _scheduleStartupMaintenance(List initialItems) { if (_startupMaintenanceScheduled) { return; @@ -170,7 +159,7 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { return; } - final persistedUpdates = >[]; + final persistedUpdates = []; var changed = false; var repairedCount = 0; var verifiedCount = 0; @@ -234,11 +223,13 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { } else { repairedCount++; } - persistedUpdates.add(updated.toJson()); + persistedUpdates.add( + HistoryMaintenanceUpdate(original: item, updated: updated), + ); } if (changed) { - await _commitMaintenanceUpdates(persistedUpdates); + await applyMaintenanceUpdates(persistedUpdates); _historyLog.i( 'SAF repair pass: verified=$verifiedCount, repaired=$repairedCount, checked=${selectedIndexes.length}', ); @@ -562,7 +553,7 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { return; } - final persistedUpdates = >[]; + final persistedUpdates = []; var refreshedCount = 0; for (final index in selectedIndexes) { @@ -693,12 +684,14 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { hasReplayGain: resolvedHasReplayGain, replayGainMetadataScanVersion: resolvedReplayGainScanVersion, ); - persistedUpdates.add(updated.toJson()); + persistedUpdates.add( + HistoryMaintenanceUpdate(original: item, updated: updated), + ); refreshedCount++; } if (persistedUpdates.isNotEmpty) { - await _commitMaintenanceUpdates(persistedUpdates); + await applyMaintenanceUpdates(persistedUpdates); } await _writeStartupCursor( diff --git a/lib/services/conversion_library_service.dart b/lib/services/conversion_library_service.dart index 5caa9a3f..f5f81087 100644 --- a/lib/services/conversion_library_service.dart +++ b/lib/services/conversion_library_service.dart @@ -1,4 +1,4 @@ -import 'package:spotiflac_android/providers/download_history_provider.dart'; +import 'package:spotiflac_android/models/download_history.dart'; import 'package:spotiflac_android/services/history_database.dart'; import 'package:spotiflac_android/services/platform_bridge.dart'; import 'package:spotiflac_android/utils/audio_conversion_utils.dart'; diff --git a/lib/services/history_database.dart b/lib/services/history_database.dart index ecf1c314..acb3dceb 100644 --- a/lib/services/history_database.dart +++ b/lib/services/history_database.dart @@ -810,6 +810,9 @@ class HistoryDatabase { }); } + Future replaceAll(Stream> items) async => + replaceHistoryRows(await database, items.map(_jsonToDbRow)); + Future> updateExistingBatch( List> items, ) async { @@ -1155,7 +1158,7 @@ class HistoryDatabase { _searchFtsAvailable = null; } - Future updateFilePath( + Future updateFilePath( String id, String newFilePath, { String? newSafFileName, @@ -1166,6 +1169,8 @@ class HistoryDatabase { int? newBitrate, String? newFormat, bool clearAudioSpecs = false, + String? expectedFilePath, + String? expectedDownloadedAt, }) async { final db = await database; final values = {'file_path': newFilePath}; @@ -1198,11 +1203,21 @@ class HistoryDatabase { values['sample_rate'] = newSampleRate; } } - await db.transaction((txn) async { - await txn.update('history', values, where: 'id = ?', whereArgs: [id]); + return db.transaction((txn) async { + final changed = await txn.update( + 'history', + values, + where: + 'id = ?' + '${expectedFilePath == null ? '' : ' AND file_path = ?'}' + '${expectedDownloadedAt == null ? '' : ' AND downloaded_at = ?'}', + whereArgs: [id, ?expectedFilePath, ?expectedDownloadedAt], + ); + if (changed == 0) return false; final batch = txn.batch(); _putPathKeysInBatch(batch, id, newFilePath); await batch.commit(noResult: true); + return true; }); } @@ -1259,6 +1274,7 @@ class HistoryDatabase { 'download_tree_uri', 'saf_relative_dir', 'saf_file_name', + 'downloaded_at', ], where: 'file_path IS NOT NULL AND file_path != ""', orderBy: 'sort_added DESC, id DESC', @@ -1268,6 +1284,36 @@ class HistoryDatabase { return rows.map((r) => Map.from(r)).toList(); } + /// A delayed absence check must not delete a replacement downloaded after + /// the inspected snapshot, even when it reused the same ID and path. + Future> deleteInspectedEntries( + List> entries, + ) async { + final db = await database; + return db.transaction((txn) async { + final deletedIds = []; + for (final entry in entries) { + final id = entry['id'] as String; + final downloadedAt = entry['downloaded_at'] as String?; + final count = await txn.delete( + 'history', + where: + 'id = ? AND file_path = ?' + '${downloadedAt == null ? '' : ' AND downloaded_at = ?'}', + whereArgs: [id, entry['file_path'], ?downloadedAt], + ); + if (count == 0) continue; + await txn.delete( + 'history_path_keys', + where: 'item_id = ?', + whereArgs: [id], + ); + deletedIds.add(id); + } + return deletedIds; + }); + } + Future deleteByIds(List ids) async { if (ids.isEmpty) return 0; diff --git a/lib/services/history_maintenance.dart b/lib/services/history_maintenance.dart index d83387f6..4132d969 100644 --- a/lib/services/history_maintenance.dart +++ b/lib/services/history_maintenance.dart @@ -1,6 +1,77 @@ import 'package:sqflite/sqflite.dart'; +import 'package:spotiflac_android/models/download_history.dart'; import 'package:spotiflac_android/services/sqlite_helpers.dart' as sqlite; +/// A delayed probe owns only fields it changed on the file it inspected. +/// Newer user edits and results for a replacement file remain authoritative. +class HistoryMaintenanceUpdate { + const HistoryMaintenanceUpdate({ + required this.original, + required this.updated, + }); + + final DownloadHistoryItem original; + final DownloadHistoryItem updated; + + DownloadHistoryItem? mergeInto(DownloadHistoryItem current) { + if (current.id != original.id || + current.filePath != original.filePath || + !current.downloadedAt.isAtSameMomentAs(original.downloadedAt)) { + return null; + } + final before = original.toJson(); + final next = current.toJson(); + final keepLyrics = + current.lyricsMetadataScanVersion != original.lyricsMetadataScanVersion; + final keepReplayGain = + current.replayGainMetadataScanVersion != + original.replayGainMetadataScanVersion; + for (final field in updated.toJson().entries) { + if (field.value == before[field.key] || + next[field.key] != before[field.key]) { + continue; + } + if (keepLyrics && + (field.key == 'hasLyrics' || + field.key == 'lyricsMetadataScanVersion')) { + continue; + } + if (keepReplayGain && + (field.key == 'hasReplayGain' || + field.key == 'replayGainMetadataScanVersion')) { + continue; + } + next[field.key] = field.value; + } + return DownloadHistoryItem.fromJson(next); + } +} + +/// Streaming restore is atomic, including failures after a flushed batch. +Future replaceHistoryRows( + Database db, + Stream> rows, +) => db.transaction((txn) async { + await txn.delete('history_path_keys'); + await txn.delete('history'); + var batch = txn.batch(); + var pending = 0; + await for (final row in rows) { + batch.insert('history', row, conflictAlgorithm: ConflictAlgorithm.replace); + sqlite.putPathKeysInBatch( + batch, + 'history_path_keys', + row['id'] as String, + row['file_path'] as String?, + ); + if (++pending < 500) continue; + await batch.commit(noResult: true); + batch = txn.batch(); + pending = 0; + } + if (pending > 0) await batch.commit(noResult: true); +}); + /// Called inside a transaction. Delayed metadata/SAF probes must never insert /// rows deleted by orphan cleanup while the probe was running. Future> updateExistingHistoryRows( diff --git a/test/history_maintenance_test.dart b/test/history_maintenance_test.dart index 0a3d3fcb..32312799 100644 --- a/test/history_maintenance_test.dart +++ b/test/history_maintenance_test.dart @@ -10,6 +10,8 @@ import 'package:spotiflac_android/providers/download_history_provider.dart'; import 'package:spotiflac_android/services/history_database.dart'; import 'package:spotiflac_android/services/history_maintenance.dart'; +import 'support/sqlite_process_database.dart'; + class _ReloadDatabase implements HistoryDatabase { _ReloadDatabase(this.rows); @@ -45,13 +47,21 @@ class _ReloadDatabase implements HistoryDatabase { return previousCount - rows.length; } + @override + Future clearAll() async { + if (failDeletion) throw StateError('database write failed'); + rows.clear(); + } + @override dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); } class _HistoryNotifier extends DownloadHistoryNotifier { - _HistoryNotifier(_ReloadDatabase database) - : _initialItems = database.rows.values.toList(), + _HistoryNotifier( + HistoryDatabase database, + Iterable items, + ) : _initialItems = items.toList(), super(database: database); final List _initialItems; @@ -63,6 +73,83 @@ class _HistoryNotifier extends DownloadHistoryNotifier { ); } +class _MutationDatabase implements HistoryDatabase { + _MutationDatabase(DownloadHistoryItem item) : rows = {item.id: item}; + + final Map rows; + final writeStarted = Completer(); + final releaseWrite = Completer(); + bool failWrite = false; + + @override + Future?> getById(String id) async => rows[id]?.toJson(); + + @override + Future upsert(Map json) async { + if (!writeStarted.isCompleted) writeStarted.complete(); + await releaseWrite.future; + if (failWrite) throw StateError('database write failed'); + final item = DownloadHistoryItem.fromJson(json); + rows[item.id] = item; + } + + @override + Future deleteByIds(List ids) async { + final previousCount = rows.length; + rows.removeWhere((id, _) => ids.contains(id)); + return previousCount - rows.length; + } + + @override + Future getCount() async => rows.length; + + @override + Future> updateExistingBatch( + List> items, + ) async { + final changedIds = {}; + for (final json in items) { + final id = json['id'] as String; + if (!rows.containsKey(id)) continue; + rows[id] = DownloadHistoryItem.fromJson(json); + changedIds.add(id); + } + return changedIds; + } + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +class _InspectionDatabase extends _MutationDatabase { + _InspectionDatabase(super.item); + + final snapshotRead = Completer(); + final releaseSnapshot = Completer(); + + @override + Future>> getEntriesWithPathsPage({ + required int limit, + int offset = 0, + }) async { + final snapshot = [ + for (final item in rows.values) + { + 'id': item.id, + 'file_path': item.filePath, + 'storage_mode': item.storageMode, + 'download_tree_uri': item.downloadTreeUri, + 'saf_relative_dir': item.safRelativeDir, + 'saf_file_name': item.safFileName, + 'downloaded_at': item.downloadedAt.toUtc().toIso8601String(), + }, + ]; + if (!snapshotRead.isCompleted) snapshotRead.complete(); + await releaseSnapshot.future; + return snapshot; + } +} + class _HistoryRows implements DatabaseExecutor { final statements = >[]; final _keys = _PathKeys(); @@ -146,7 +233,9 @@ void main() { final db = _ReloadDatabase({item.id: item}); final container = ProviderContainer( overrides: [ - downloadHistoryProvider.overrideWith(() => _HistoryNotifier(db)), + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), ], ); addTearDown(container.dispose); @@ -174,7 +263,9 @@ void main() { final db = _ReloadDatabase({item.id: item})..failDeletion = true; final container = ProviderContainer( overrides: [ - downloadHistoryProvider.overrideWith(() => _HistoryNotifier(db)), + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), ], ); addTearDown(container.dispose); @@ -190,6 +281,271 @@ void main() { }, ); + for (final deletion in ['single', 'many', 'clear']) { + Future remove(DownloadHistoryNotifier notifier, String id) => + switch (deletion) { + 'single' => notifier.removeFromHistory(id), + 'many' => notifier.removeManyFromHistory([id]), + _ => notifier.clearHistory(), + }; + + test('$deletion deletion waits for an older reload', () async { + final item = _track('removed'); + final db = _ReloadDatabase({item.id: item}); + final container = ProviderContainer( + overrides: [ + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), + ], + ); + addTearDown(container.dispose); + final notifier = container.read(downloadHistoryProvider.notifier); + final reload = notifier.reloadFromStorage(); + await db.snapshotRead.future; + final deletion = remove(notifier, item.id); + await Future.delayed(Duration.zero); + expect(db.rows.keys, [item.id]); + db.count.complete(1); + await Future.wait([reload, deletion]); + final state = container.read(downloadHistoryProvider); + expect(db.rows, isEmpty); + expect(state.items, isEmpty); + expect(state.lookupItems, isEmpty); + expect(state.totalCount, 0); + }); + + test('failed $deletion deletion keeps the visible record', () async { + final item = _track('retained'); + final db = _ReloadDatabase({item.id: item})..failDeletion = true; + final container = ProviderContainer( + overrides: [ + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), + ], + ); + addTearDown(container.dispose); + await remove(container.read(downloadHistoryProvider.notifier), item.id); + final state = container.read(downloadHistoryProvider); + expect(db.rows.keys, [item.id]); + expect(state.items, [item]); + expect(state.totalCount, 1); + expect(state.loadedIndexVersion, 0); + }); + } + + for (final audioOnly in [false, true]) { + Future update(DownloadHistoryNotifier notifier, String id) => + audioOnly + ? notifier.updateAudioMetadataForItem(id: id, quality: '24-bit/96kHz') + : notifier.updateMetadataForItem( + id: id, + trackName: 'Updated', + artistName: 'Artist', + albumName: 'Album', + ); + + test('failed metadata save keeps state (audio: $audioOnly)', () async { + final item = _track('retained'); + final db = _MutationDatabase(item)..failWrite = true; + db.releaseWrite.complete(); + final container = ProviderContainer( + overrides: [ + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), + ], + ); + addTearDown(container.dispose); + await expectLater( + update(container.read(downloadHistoryProvider.notifier), item.id), + throwsStateError, + ); + final state = container.read(downloadHistoryProvider); + expect(db.rows.values, [item]); + expect(state.items, [item]); + expect(state.lookupItems, [item]); + expect(state.loadedIndexVersion, 0); + }); + + test( + 'metadata cannot revive a deleted record (audio: $audioOnly)', + () async { + final item = _track('removed'); + final db = _MutationDatabase(item); + final container = ProviderContainer( + overrides: [ + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), + ], + ); + addTearDown(container.dispose); + final notifier = container.read(downloadHistoryProvider.notifier); + final updating = update(notifier, item.id); + await db.writeStarted.future; + final deleting = notifier.removeFromHistory(item.id); + await Future.delayed(Duration.zero); + expect(db.rows.keys, [item.id]); + expect(container.read(downloadHistoryProvider).items, [item]); + db.releaseWrite.complete(); + await Future.wait([updating, deleting]); + final state = container.read(downloadHistoryProvider); + expect(db.rows, isEmpty); + expect(state.items, isEmpty); + expect(state.lookupItems, isEmpty); + expect(state.totalCount, 0); + }, + ); + } + + test('delayed maintenance preserves newer metadata edits', () async { + final original = _track('kept').copyWith(composer: 'Old composer'); + final db = _MutationDatabase(original); + db.releaseWrite.complete(); + final container = ProviderContainer( + overrides: [ + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), + ], + ); + addTearDown(container.dispose); + final notifier = container.read(downloadHistoryProvider.notifier); + await notifier.updateMetadataForItem( + id: original.id, + trackName: 'User title', + artistName: original.artistName, + albumName: original.albumName, + composer: 'User composer', + ); + await notifier.applyMaintenanceUpdates([ + HistoryMaintenanceUpdate( + original: original, + updated: original.copyWith( + quality: '24-bit/96kHz', + composer: 'Probed composer', + filePath: '/music/repaired.flac', + ), + ), + ]); + final stored = db.rows[original.id]!; + final visible = container.read(downloadHistoryProvider).items.single; + for (final item in [stored, visible]) { + expect(item.trackName, 'User title'); + expect(item.composer, 'User composer'); + expect(item.quality, '24-bit/96kHz'); + expect(item.filePath, '/music/repaired.flac'); + } + }); + + test('newer tag scan results survive an older metadata probe', () { + final original = _track('kept'); + final current = original.copyWith( + lyricsMetadataScanVersion: 2, + replayGainMetadataScanVersion: 2, + ); + final merged = HistoryMaintenanceUpdate( + original: original, + updated: original.copyWith( + hasLyrics: true, + lyricsMetadataScanVersion: 1, + hasReplayGain: true, + replayGainMetadataScanVersion: 1, + ), + ).mergeInto(current)!; + expect(merged.hasLyrics, isFalse); + expect(merged.hasReplayGain, isFalse); + expect(merged.lyricsMetadataScanVersion, 2); + expect(merged.replayGainMetadataScanVersion, 2); + }); + + for (final replacedPath in [false, true]) { + test('old orphan inspection keeps a replacement ($replacedPath)', () async { + final original = _track('replaced'); + final db = _InspectionDatabase(original); + db.releaseWrite.complete(); + final container = ProviderContainer( + overrides: [ + downloadHistoryProvider.overrideWith( + () => _HistoryNotifier(db, db.rows.values), + ), + ], + ); + addTearDown(container.dispose); + final notifier = container.read(downloadHistoryProvider.notifier); + final cleanup = notifier.cleanupOrphanedDownloads(); + await db.snapshotRead.future; + final replacement = DownloadHistoryItem.fromJson({ + ...original.toJson(), + 'filePath': replacedPath ? '/music/new-file.flac' : original.filePath, + 'downloadedAt': original.downloadedAt + .add(const Duration(days: 1)) + .toUtc() + .toIso8601String(), + }); + await notifier.addToHistory(replacement); + db.releaseSnapshot.complete(); + expect(await cleanup, 0); + expect(db.rows[original.id]!.filePath, replacement.filePath); + expect( + container.read(downloadHistoryProvider).items.single.filePath, + replacement.filePath, + ); + expect(container.read(downloadHistoryProvider).totalCount, 1); + }); + } + + test('atomic streaming restore rolls back a late stream failure', () async { + final db = await SqliteProcessDatabase.open(); + addTearDown(db.close); + await db.execute( + 'CREATE TABLE history(id TEXT PRIMARY KEY, file_path TEXT NOT NULL)', + ); + await db.execute( + 'CREATE TABLE history_path_keys(item_id TEXT, path_key TEXT, ' + 'PRIMARY KEY(item_id, path_key))', + ); + await db.insert('history', { + 'id': 'previous', + 'file_path': '/music/old.flac', + }); + await db.insert('history_path_keys', { + 'item_id': 'previous', + 'path_key': 'old-key', + }); + Stream> brokenBackup() async* { + for (var i = 0; i < 501; i++) { + yield {'id': '$i', 'file_path': '/music/$i.flac'}; + } + throw const FormatException('truncated backup'); + } + + await expectLater( + replaceHistoryRows(db, brokenBackup()), + throwsFormatException, + ); + expect(await db.query('history'), [ + {'id': 'previous', 'file_path': '/music/old.flac'}, + ]); + expect(await db.query('history_path_keys'), [ + {'item_id': 'previous', 'path_key': 'old-key'}, + ]); + await replaceHistoryRows( + db, + Stream.fromIterable([ + {'id': 'restored', 'file_path': '/music/restored.flac'}, + ]), + ); + expect(await db.query('history'), [ + {'id': 'restored', 'file_path': '/music/restored.flac'}, + ]); + final keys = await db.query('history_path_keys'); + expect(keys, isNotEmpty); + expect(keys.every((row) => row['item_id'] == 'restored'), isTrue); + }); + test( 'late maintenance cannot recreate deleted rows or their path keys', () async {