Fix #4296 + download UI improvement + fixes (#4369)

## Description

### Background 
Proposal to fix #4296 (@ua741). High level, there is a race condition
which is generating wrong entries in the FilesDB (creating orphan
files). This issue is easily replicable if you select multiple files and
try to download them (the more files, the more likely wrong entries will
be created). This issue is also messing up with the date of the files we
see in the timeline and is creating "orphan" (not linked to uploaded
files) files in the device collections.

To my understanding, this is quite a critical bug and any user is very
likely to encounter it in the current implementation.

There is some self healing already in place that will reupload all those
files and this should correct the dates, but this will only do it 10
files at a time, and based on potential candidates, without fixing the
orphan entries. I was also able to produce files which were never
corrected.

### This pull request
After extensive tests and lots of debugging, I was able to fully fix
this issue (meaning I am not able to reproduce it, at all, and I tried a
lot..). I also do not see any orphan in the device collection
("Pictures" folder for Android), while i could see 20+ (even more) each
time I was trying to replicate the issue. To fix it, I have:

- created a new class `PhotoManagerSafe` which is safely making sure
that the app will not react to file changes when it has been asked not
to do so (previous implementation allowed scenarios where a caller was
executing code thinking no change notification would be sent).
- created a Lock in the `LocalSyncService` that is used to make sure no
local synchronisation can be performed at the same time a download is
performed (this was the main culprit). The main side effect is that when
you download multiple files, they will be processed sequentially. I
think this can be improved but the main "side" advantage is that
whenever there is a failure, not a lot will be lost and it is more
robust in that way if you want to download a big collection. It would be
better to have at least 2 threads downloading/decrypting at the same
time, maybe (maybe not needed as well)?
- improved the "Downloading files..." dialog, which is now showing the
progress (for example "Downloading files... (235/494)"), so that the
user knows if something is actually happening and what is the current
progress.
- Added a missing call on the Video Editor page where the change notify
was stopped but never started again at the end when task was performed.

Feel free to close this PR if not suitable according to you, I can also
take feedback and try to implement them as best as I can.

## Tests

Tested on my Pixel 6a

Built with :

Flutter 3.24.3
JDK 17.0.2
Gradle 7.2
This commit is contained in:
Neeraj Gupta
2024-12-13 14:17:34 +05:30
committed by GitHub
4 changed files with 142 additions and 76 deletions

View File

@@ -23,6 +23,7 @@ import "package:photos/utils/debouncer.dart";
import "package:photos/utils/photo_manager_util.dart";
import "package:photos/utils/sqlite_util.dart";
import 'package:shared_preferences/shared_preferences.dart';
import 'package:synchronized/synchronized.dart';
import 'package:tuple/tuple.dart';
class LocalSyncService {
@@ -31,6 +32,7 @@ class LocalSyncService {
late SharedPreferences _prefs;
Completer<void>? _existingSync;
late Debouncer _changeCallbackDebouncer;
final Lock _lock = Lock();
static const kDbUpdationTimeKey = "db_updation_time";
static const kHasCompletedFirstImportKey = "has_completed_firstImport";
@@ -77,50 +79,57 @@ class LocalSyncService {
}
_existingSync = Completer<void>();
final int ownerID = Configuration.instance.getUserID()!;
final existingLocalFileIDs = await _db.getExistingLocalFileIDs(ownerID);
_logger.info("${existingLocalFileIDs.length} localIDs were discovered");
// We use a lock to prevent synchronisation to occur while it is downloading
// as this introduces wrong entry in FilesDB due to race condition
// This is a fix for https://github.com/ente-io/ente/issues/4296
await _lock.synchronized(() async {
final existingLocalFileIDs = await _db.getExistingLocalFileIDs(ownerID);
_logger.info("${existingLocalFileIDs.length} localIDs were discovered");
final syncStartTime = DateTime.now().microsecondsSinceEpoch;
final lastDBUpdationTime = _prefs.getInt(kDbUpdationTimeKey) ?? 0;
final startTime = DateTime.now().microsecondsSinceEpoch;
if (lastDBUpdationTime != 0) {
await _loadAndStoreDiff(
existingLocalFileIDs,
fromTime: lastDBUpdationTime,
toTime: syncStartTime,
);
} else {
// Load from 0 - 01.01.2010
Bus.instance.fire(SyncStatusUpdate(SyncStatus.startedFirstGalleryImport));
var startTime = 0;
var toYear = 2010;
var toTime = DateTime(toYear).microsecondsSinceEpoch;
while (toTime < syncStartTime) {
final syncStartTime = DateTime.now().microsecondsSinceEpoch;
final lastDBUpdationTime = _prefs.getInt(kDbUpdationTimeKey) ?? 0;
final startTime = DateTime.now().microsecondsSinceEpoch;
if (lastDBUpdationTime != 0) {
await _loadAndStoreDiff(
existingLocalFileIDs,
fromTime: lastDBUpdationTime,
toTime: syncStartTime,
);
} else {
// Load from 0 - 01.01.2010
Bus.instance.fire(SyncStatusUpdate(SyncStatus.startedFirstGalleryImport));
var startTime = 0;
var toYear = 2010;
var toTime = DateTime(toYear).microsecondsSinceEpoch;
while (toTime < syncStartTime) {
await _loadAndStoreDiff(
existingLocalFileIDs,
fromTime: startTime,
toTime: toTime,
);
startTime = toTime;
toYear++;
toTime = DateTime(toYear).microsecondsSinceEpoch;
}
await _loadAndStoreDiff(
existingLocalFileIDs,
fromTime: startTime,
toTime: toTime,
toTime: syncStartTime,
);
startTime = toTime;
toYear++;
toTime = DateTime(toYear).microsecondsSinceEpoch;
}
await _loadAndStoreDiff(
existingLocalFileIDs,
fromTime: startTime,
toTime: syncStartTime,
);
}
if (!hasCompletedFirstImport()) {
await _prefs.setBool(kHasCompletedFirstImportKey, true);
await _refreshDeviceFolderCountAndCover(isFirstSync: true);
_logger.fine("first gallery import finished");
Bus.instance
.fire(SyncStatusUpdate(SyncStatus.completedFirstGalleryImport));
}
final endTime = DateTime.now().microsecondsSinceEpoch;
final duration = Duration(microseconds: endTime - startTime);
_logger.info("Load took " + duration.inMilliseconds.toString() + "ms");
if (!hasCompletedFirstImport()) {
await _prefs.setBool(kHasCompletedFirstImportKey, true);
await _refreshDeviceFolderCountAndCover(isFirstSync: true);
_logger.fine("first gallery import finished");
Bus.instance
.fire(SyncStatusUpdate(SyncStatus.completedFirstGalleryImport));
}
final endTime = DateTime.now().microsecondsSinceEpoch;
final duration = Duration(microseconds: endTime - startTime);
_logger.info("Load took " + duration.inMilliseconds.toString() + "ms");
});
_existingSync?.complete();
_existingSync = null;
}
@@ -240,6 +249,10 @@ class LocalSyncService {
}
}
Lock getLock() {
return _lock;
}
bool hasGrantedPermissions() {
return _prefs.getBool(kHasGrantedPermissionsKey) ?? false;
}

View File

@@ -299,6 +299,8 @@ class _VideoEditorPageState extends State<VideoEditorPage> {
);
} catch (_) {
await dialog.hide();
} finally {
await PhotoManager.startChangeNotify();
}
}
}

View File

@@ -822,17 +822,27 @@ class _FileSelectionActionsWidgetState
}
Future<void> _download(List<EnteFile> files) async {
final totalFiles = files.length;
int downloadedFiles = 0;
final dialog = createProgressDialog(
context,
S.of(context).downloading,
S.of(context).downloading + " ($downloadedFiles/$totalFiles)",
isDismissible: true,
);
await dialog.show();
try {
final downloadQueue = DownloadQueue(maxConcurrent: 5);
final futures = <Future>[];
for (final file in files) {
if (file.localID == null) {
futures.add(downloadToGallery(file));
futures.add(
downloadQueue.add(() async {
await downloadToGallery(file);
downloadedFiles++;
dialog.update(message: S.of(context).downloading + " ($downloadedFiles/$totalFiles)");
}),
);
}
}
await Future.wait(futures);

View File

@@ -1,3 +1,5 @@
import "dart:async";
import "dart:collection";
import 'dart:io';
import 'package:dio/dio.dart';
@@ -190,44 +192,49 @@ Future<void> downloadToGallery(EnteFile file) async {
type == FileType.livePhoto && Platform.isAndroid;
AssetEntity? savedAsset;
final File? fileToSave = await getFile(file);
//Disabling notifications for assets changing to insert the file into
//files db before triggering a sync.
await PhotoManager.stopChangeNotify();
if (type == FileType.image) {
savedAsset = await PhotoManager.editor
.saveImageWithPath(fileToSave!.path, title: file.title!);
} else if (type == FileType.video) {
savedAsset =
await PhotoManager.editor.saveVideo(fileToSave!, title: file.title!);
} else if (type == FileType.livePhoto) {
final File? liveVideoFile =
await getFileFromServer(file, liveVideo: true);
if (liveVideoFile == null) {
throw AssertionError("Live video can not be null");
// We use a lock to prevent synchronisation to occur while it is downloading
// as this introduces wrong entry in FilesDB due to race condition
// This is a fix for https://github.com/ente-io/ente/issues/4296
await LocalSyncService.instance.getLock().synchronized(() async {
//Disabling notifications for assets changing to insert the file into
//files db before triggering a sync.
await PhotoManager.stopChangeNotify();
if (type == FileType.image) {
savedAsset = await PhotoManager.editor
.saveImageWithPath(fileToSave!.path, title: file.title!);
} else if (type == FileType.video) {
savedAsset =
await PhotoManager.editor.saveVideo(fileToSave!, title: file.title!);
} else if (type == FileType.livePhoto) {
final File? liveVideoFile =
await getFileFromServer(file, liveVideo: true);
if (liveVideoFile == null) {
throw AssertionError("Live video can not be null");
}
if (downloadLivePhotoOnDroid) {
await _saveLivePhotoOnDroid(fileToSave!, liveVideoFile, file);
} else {
savedAsset = await PhotoManager.editor.darwin.saveLivePhoto(
imageFile: fileToSave!,
videoFile: liveVideoFile,
title: file.title!,
);
}
}
if (downloadLivePhotoOnDroid) {
await _saveLivePhotoOnDroid(fileToSave!, liveVideoFile, file);
} else {
savedAsset = await PhotoManager.editor.darwin.saveLivePhoto(
imageFile: fileToSave!,
videoFile: liveVideoFile,
title: file.title!,
);
}
}
if (savedAsset != null) {
file.localID = savedAsset.id;
await FilesDB.instance.insert(file);
Bus.instance.fire(
LocalPhotosUpdatedEvent(
[file],
source: "download",
),
);
} else if (!downloadLivePhotoOnDroid && savedAsset == null) {
_logger.severe('Failed to save assert of type $type');
}
if (savedAsset != null) {
file.localID = savedAsset!.id;
await FilesDB.instance.insert(file);
Bus.instance.fire(
LocalPhotosUpdatedEvent(
[file],
source: "download",
),
);
} else if (!downloadLivePhotoOnDroid && savedAsset == null) {
_logger.severe('Failed to save assert of type $type');
}
});
} catch (e) {
_logger.severe("Failed to save file", e);
rethrow;
@@ -273,3 +280,37 @@ Future<void> _saveLivePhotoOnDroid(
);
await IgnoredFilesService.instance.cacheAndInsert([ignoreVideoFile]);
}
class DownloadQueue {
final int maxConcurrent;
final Queue<Future<void> Function()> _queue = Queue();
int _runningTasks = 0;
DownloadQueue({this.maxConcurrent = 5});
Future<void> add(Future<void> Function() task) async {
final completer = Completer<void>();
_queue.add(() async {
try {
await task();
completer.complete();
} catch (e) {
completer.completeError(e);
} finally {
_runningTasks--;
_processQueue();
}
return completer.future;
});
_processQueue();
return completer.future;
}
void _processQueue() {
while (_runningTasks < maxConcurrent && _queue.isNotEmpty) {
final task = _queue.removeFirst();
_runningTasks++;
task();
}
}
}