diff --git a/lib/providers/download_history_provider.dart b/lib/providers/download_history_provider.dart index 16b8f042..77990f93 100644 --- a/lib/providers/download_history_provider.dart +++ b/lib/providers/download_history_provider.dart @@ -19,6 +19,27 @@ part 'download_history_provider_maintenance.dart'; final _historyLog = AppLogger('DownloadHistory'); +/// Merge a delayed maintenance result into the live lists, never its old +/// snapshot. Deletions, new downloads, ordering and counts stay authoritative. +DownloadHistoryState mergeHistoryMaintenanceUpdates( + DownloadHistoryState current, + Iterable updates, + Set persistedIds, +) { + final byId = { + for (final item in updates) + if (persistedIds.contains(item.id)) item.id: item, + }; + if (byId.isEmpty) return current; + return current.copyWith( + items: [for (final item in current.items) byId[item.id] ?? item], + lookupItems: [ + for (final item in current.lookupItems) byId[item.id] ?? item, + ], + loadedIndexVersion: current.loadedIndexVersion + 1, + ); +} + typedef StartupOrphanDecision = ({ Set confirmedIds, Set pendingIds, diff --git a/lib/providers/download_history_provider_maintenance.dart b/lib/providers/download_history_provider_maintenance.dart index c3801bfd..e324eab2 100644 --- a/lib/providers/download_history_provider_maintenance.dart +++ b/lib/providers/download_history_provider_maintenance.dart @@ -4,6 +4,17 @@ 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; @@ -159,7 +170,6 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { return; } - final updatedItems = [...items]; final persistedUpdates = >[]; var changed = false; var repairedCount = 0; @@ -218,7 +228,6 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { : item.safFileName, safRepaired: true, ); - updatedItems[index] = updated; changed = true; if (newUri == item.filePath) { verifiedCount++; @@ -229,12 +238,7 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { } if (changed) { - await _db.upsertBatch(persistedUpdates); - state = state.copyWith( - items: updatedItems, - loadedIndexVersion: state.loadedIndexVersion + 1, - lookupItems: _lookupItemsWithUpdates(updatedItems), - ); + await _commitMaintenanceUpdates(persistedUpdates); _historyLog.i( 'SAF repair pass: verified=$verifiedCount, repaired=$repairedCount, checked=${selectedIndexes.length}', ); @@ -558,7 +562,6 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { return; } - List? updatedItems; final persistedUpdates = >[]; var refreshedCount = 0; @@ -690,19 +693,12 @@ extension _HistoryStartupMaintenance on DownloadHistoryNotifier { hasReplayGain: resolvedHasReplayGain, replayGainMetadataScanVersion: resolvedReplayGainScanVersion, ); - updatedItems ??= [...items]; - updatedItems[index] = updated; persistedUpdates.add(updated.toJson()); refreshedCount++; } - if (persistedUpdates.isNotEmpty && updatedItems != null) { - await _db.upsertBatch(persistedUpdates); - state = state.copyWith( - items: updatedItems, - loadedIndexVersion: state.loadedIndexVersion + 1, - lookupItems: _lookupItemsWithUpdates(updatedItems), - ); + if (persistedUpdates.isNotEmpty) { + await _commitMaintenanceUpdates(persistedUpdates); } await _writeStartupCursor( diff --git a/lib/services/history_database.dart b/lib/services/history_database.dart index e6d0d77b..9accdb0b 100644 --- a/lib/services/history_database.dart +++ b/lib/services/history_database.dart @@ -4,6 +4,7 @@ import 'package:sqflite/sqflite.dart'; import 'package:path_provider/path_provider.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:spotiflac_android/services/sqlite_helpers.dart' as sqlite; +import 'package:spotiflac_android/services/history_maintenance.dart'; import 'package:spotiflac_android/utils/isrc_utils.dart' as isrc; import 'package:spotiflac_android/utils/ios_container_paths.dart'; import 'package:spotiflac_android/utils/logger.dart'; @@ -806,6 +807,16 @@ class HistoryDatabase { }); } + Future> updateExistingBatch( + List> items, + ) async { + if (items.isEmpty) return const {}; + final db = await database; + return db.transaction( + (txn) => updateExistingHistoryRows(txn, items.map(_jsonToDbRow)), + ); + } + Future>> getAll({int? limit, int? offset}) async { final db = await database; final rows = await db.query( diff --git a/lib/services/history_maintenance.dart b/lib/services/history_maintenance.dart new file mode 100644 index 00000000..d83387f6 --- /dev/null +++ b/lib/services/history_maintenance.dart @@ -0,0 +1,31 @@ +import 'package:sqflite/sqflite.dart'; +import 'package:spotiflac_android/services/sqlite_helpers.dart' as sqlite; + +/// Called inside a transaction. Delayed metadata/SAF probes must never insert +/// rows deleted by orphan cleanup while the probe was running. +Future> updateExistingHistoryRows( + DatabaseExecutor db, + Iterable> rows, +) async { + final updatedIds = {}; + final batch = db.batch(); + for (final row in rows) { + final id = row['id'] as String; + final changed = await db.update( + 'history', + row, + where: 'id = ?', + whereArgs: [id], + ); + if (changed == 0) continue; + updatedIds.add(id); + sqlite.putPathKeysInBatch( + batch, + 'history_path_keys', + id, + row['file_path'] as String?, + ); + } + if (updatedIds.isNotEmpty) await batch.commit(noResult: true); + return updatedIds; +} diff --git a/test/history_maintenance_test.dart b/test/history_maintenance_test.dart new file mode 100644 index 00000000..968e6b03 --- /dev/null +++ b/test/history_maintenance_test.dart @@ -0,0 +1,186 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:sqflite/sqflite.dart'; +import 'package:spotiflac_android/providers/download_history_provider.dart'; +import 'package:spotiflac_android/services/history_maintenance.dart'; + +class _HistoryRows implements DatabaseExecutor { + final statements = >[]; + final _keys = _PathKeys(); + + @override + Future update( + String table, + Map values, { + String? where, + List? whereArgs, + ConflictAlgorithm? conflictAlgorithm, + }) async { + statements.add({ + 'sql': + 'UPDATE $table SET ${values.keys.map((key) => '$key = ?').join(', ')} WHERE $where', + 'args': [...values.values, ...?whereArgs], + }); + return whereArgs!.single == 'kept' ? 1 : 0; + } + + @override + Batch batch() => _keys; + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +class _PathKeys implements Batch { + final statements = >[]; + + @override + void delete(String table, {String? where, List? whereArgs}) { + statements.add({ + 'sql': 'DELETE FROM $table WHERE $where', + 'args': whereArgs, + }); + } + + @override + void insert( + String table, + Map values, { + String? nullColumnHack, + ConflictAlgorithm? conflictAlgorithm, + }) { + expect(conflictAlgorithm, ConflictAlgorithm.ignore); + statements.add({ + 'sql': + 'INSERT OR IGNORE INTO $table (${values.keys.join(', ')}) VALUES (${values.keys.map((_) => '?').join(', ')})', + 'args': values.values.toList(), + }); + } + + @override + Future> commit({ + bool? exclusive, + bool? noResult, + bool? continueOnError, + }) async => []; + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +DownloadHistoryItem _track(String id) => DownloadHistoryItem( + id: id, + trackName: 'Song $id', + artistName: 'Artist', + albumName: 'Album', + filePath: '/music/$id.flac', + service: 'test', + downloadedAt: DateTime.utc(2026), +); + +void main() { + test( + 'late maintenance cannot recreate deleted rows or their path keys', + () async { + final db = _HistoryRows(); + final updated = await updateExistingHistoryRows(db, [ + { + 'id': 'removed', + 'file_path': '/music/removed.flac', + 'quality': '24-bit', + }, + {'id': 'kept', 'file_path': '/music/kept.opus', 'quality': 'Opus'}, + ]); + expect(updated, {'kept'}); + final result = await Process.run('python3', [ + '-c', + r''' +import json, sqlite3, sys +db = sqlite3.connect(':memory:') +db.executescript(""" +CREATE TABLE history(id TEXT PRIMARY KEY, file_path TEXT, quality TEXT); +CREATE TABLE history_path_keys(item_id TEXT, path_key TEXT, UNIQUE(item_id,path_key)); +INSERT INTO history VALUES ('removed','/music/removed.flac','16-bit'),('kept','/music/kept.flac','16-bit'); +INSERT INTO history_path_keys SELECT id,file_path FROM history; +DELETE FROM history WHERE id='removed'; +DELETE FROM history_path_keys WHERE item_id='removed'; +""") +with db: + for statement in json.loads(sys.argv[1]): + db.execute(statement['sql'], statement['args']) +print(json.dumps({ + 'rows': list(db.execute('SELECT id,file_path,quality FROM history')), + 'keys': list(db.execute('SELECT item_id,path_key FROM history_path_keys')) +})) +''', + jsonEncode([...db.statements, ...db._keys.statements]), + ]); + expect(result.exitCode, 0, reason: result.stderr.toString()); + final data = jsonDecode(result.stdout as String) as Map; + expect(data['rows'], [ + ['kept', '/music/kept.opus', 'Opus'], + ]); + expect( + (data['keys'] as List).every((row) => (row as List).first == 'kept'), + isTrue, + ); + expect(data['keys'], isNotEmpty); + }, + ); + + test( + '20 removed downloads stay removed when an old metadata probe finishes', + () { + final oldSnapshot = [for (var i = 0; i < 20; i++) _track('$i')]; + final fresh = _track('new-download'); + final current = DownloadHistoryState( + items: [fresh], + totalCount: 1, + loadedIndexVersion: 7, + ); + final result = mergeHistoryMaintenanceUpdates(current, [ + oldSnapshot.last.copyWith(quality: '24-bit'), + ], {}); + expect(identical(result, current), isTrue); + expect(result.items.map((item) => item.id), ['new-download']); + expect(result.lookupItems.map((item) => item.id), ['new-download']); + expect(result.totalCount, 1); + }, + ); + + test( + 'live ordering, new downloads and lookup-only items survive maintenance', + () { + final kept = _track('kept'); + final fresh = _track('fresh'); + final lookupOnly = _track('lookup'); + final current = DownloadHistoryState( + items: [fresh, kept], + lookupItems: [lookupOnly, fresh, kept], + totalCount: 42, + loadedIndexVersion: 3, + ); + final result = mergeHistoryMaintenanceUpdates( + current, + [ + kept.copyWith(quality: '24-bit'), + lookupOnly.copyWith(quality: 'Opus'), + _track('deleted'), + ], + {'kept', 'lookup'}, + ); + expect(result.items.map((item) => item.id), ['fresh', 'kept']); + expect(result.items.last.quality, '24-bit'); + expect(result.lookupItems.map((item) => item.id), [ + 'lookup', + 'fresh', + 'kept', + ]); + expect(result.lookupItems.first.quality, 'Opus'); + expect(result.totalCount, 42); + expect(result.loadedIndexVersion, 4); + }, + ); +}