From 8d918a65ef354dc8d47773e0cde3df8da180f3e7 Mon Sep 17 00:00:00 2001 From: Santo Shakil Date: Mon, 11 May 2026 21:03:57 +0600 Subject: [PATCH] fix(mobile): server side check in manageLinkedAlbums + wire to organize mobile cache only trusts SyncStream events. if the server looses an album silently (audit table empty) manage never spots the stale id. now fetch getAllOwned first, prune the cached row, then recreate or relink by name. also the organize button only ran sync passes before. now it runs manageLinkedAlbums too so toggle off and on actually reconciles the cache. added unit tests for both paths. verifed on pixel 9a with a sql soft delete on a local v3 server, audit row stays empty just like users 'clean reinstall' case. --- .../services/sync_linked_album.service.dart | 73 ++++--- .../drift_album_api_repository.dart | 5 + .../drift_backup_settings.dart | 1 + .../sync_linked_album_service_test.dart | 186 ++++++++++++++++++ 4 files changed, 235 insertions(+), 30 deletions(-) create mode 100644 mobile/test/domain/services/sync_linked_album_service_test.dart diff --git a/mobile/lib/domain/services/sync_linked_album.service.dart b/mobile/lib/domain/services/sync_linked_album.service.dart index 2f0004c505..493a090cf1 100644 --- a/mobile/lib/domain/services/sync_linked_album.service.dart +++ b/mobile/lib/domain/services/sync_linked_album.service.dart @@ -1,4 +1,5 @@ import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:immich_mobile/domain/models/album/album.model.dart'; import 'package:immich_mobile/domain/models/album/local_album.model.dart'; import 'package:immich_mobile/domain/models/store.model.dart'; import 'package:immich_mobile/domain/services/store.service.dart'; @@ -76,50 +77,62 @@ class SyncLinkedAlbumService { } Future manageLinkedAlbums(List localAlbums, String ownerId) async { + // fetch the server's authoritative owned-album list once and reconcile each + // local album against it. trusting only the local cache (previous behaviour) + // misses the case where the server lost an album that mobile still has + // cached (volume reset, soft-deleted user, etc). + final List serverAlbums; + try { + serverAlbums = await _albumApiRepository.getAllOwned(_storeService.get(StoreKey.currentUser)); + } catch (error, stackTrace) { + // soft-fail on network / server error so a flaky link doesn't destroy local state + _log.severe("Could not fetch server albums; deferring manageLinkedAlbums", error, stackTrace); + return; + } + + final serverById = {for (final a in serverAlbums) a.id: a}; + final serverByName = {for (final a in serverAlbums) a.name: a}; + try { for (final album in localAlbums) { - await _processLocalAlbum(album, ownerId); + await _processLocalAlbum(album, serverById, serverByName); } } catch (error, stackTrace) { _log.severe("Error managing linked albums", error, stackTrace); } } - /// Processes a single local album to ensure proper linking with remote albums - Future _processLocalAlbum(LocalAlbum localAlbum, String ownerId) { - final hasLinkedRemoteAlbum = localAlbum.linkedRemoteAlbumId != null; + /// Reconciles a single local album against the server's owned-album list. + Future _processLocalAlbum( + LocalAlbum localAlbum, + Map serverById, + Map serverByName, + ) async { + final linkedId = localAlbum.linkedRemoteAlbumId; + if (linkedId != null && serverById.containsKey(linkedId)) { + return; + } + if (linkedId != null) { + // server doesn't have this album anymore. drop the cached row; KeyAction.setNull + // on LocalAlbumEntity.linkedRemoteAlbumId nulls the link via FK cascade. + await _remoteAlbumRepository.deleteAlbum(linkedId); + } - if (hasLinkedRemoteAlbum) { - return _handleLinkedAlbum(localAlbum); + final byNameMatch = serverByName[localAlbum.name]; + if (byNameMatch != null) { + await _linkToExistingRemoteAlbum(localAlbum, byNameMatch); } else { - return _handleUnlinkedAlbum(localAlbum, ownerId); + await _createAndLinkNewRemoteAlbum(localAlbum); } } - /// Handles albums that are already linked to a remote album - Future _handleLinkedAlbum(LocalAlbum localAlbum) async { - final remoteAlbumId = localAlbum.linkedRemoteAlbumId!; - final remoteAlbum = await _remoteAlbumRepository.get(remoteAlbumId); - - final remoteAlbumExists = remoteAlbum != null; - if (!remoteAlbumExists) { - return _localAlbumRepository.unlinkRemoteAlbum(localAlbum.id); + /// Links a local album to an existing remote album, ensuring the cache row exists + /// so subsequent [syncLinkedAlbums] passes can find it without waiting for sync stream. + Future _linkToExistingRemoteAlbum(LocalAlbum localAlbum, RemoteAlbum existingRemoteAlbum) async { + final cached = await _remoteAlbumRepository.get(existingRemoteAlbum.id); + if (cached == null) { + await _remoteAlbumRepository.create(existingRemoteAlbum, []); } - } - - /// Handles albums that are not linked to any remote album - Future _handleUnlinkedAlbum(LocalAlbum localAlbum, String ownerId) async { - final existingRemoteAlbum = await _remoteAlbumRepository.getByName(localAlbum.name, ownerId); - - if (existingRemoteAlbum != null) { - return _linkToExistingRemoteAlbum(localAlbum, existingRemoteAlbum); - } else { - return _createAndLinkNewRemoteAlbum(localAlbum); - } - } - - /// Links a local album to an existing remote album - Future _linkToExistingRemoteAlbum(LocalAlbum localAlbum, dynamic existingRemoteAlbum) { return _localAlbumRepository.linkRemoteAlbum(localAlbum.id, existingRemoteAlbum.id); } diff --git a/mobile/lib/repositories/drift_album_api_repository.dart b/mobile/lib/repositories/drift_album_api_repository.dart index 2ae6e23891..0ea41c815b 100644 --- a/mobile/lib/repositories/drift_album_api_repository.dart +++ b/mobile/lib/repositories/drift_album_api_repository.dart @@ -15,6 +15,11 @@ class DriftAlbumApiRepository extends ApiRepository { DriftAlbumApiRepository(this._api); + Future> getAllOwned(UserDto owner) async { + final response = await checkNull(_api.getAllAlbums(isOwned: true)); + return response.map((dto) => dto.toRemoteAlbum(owner)).toList(); + } + Future createDriftAlbum( String name, UserDto owner, { diff --git a/mobile/lib/widgets/settings/backup_settings/drift_backup_settings.dart b/mobile/lib/widgets/settings/backup_settings/drift_backup_settings.dart index 2c179c42ea..a10a861161 100644 --- a/mobile/lib/widgets/settings/backup_settings/drift_backup_settings.dart +++ b/mobile/lib/widgets/settings/backup_settings/drift_backup_settings.dart @@ -69,6 +69,7 @@ class _AlbumSyncActionButtonState extends ConsumerState<_AlbumSyncActionButton> }); try { + await _manageLinkedAlbums(); await ref.read(backgroundSyncProvider).syncLinkedAlbum(); await ref.read(backgroundSyncProvider).syncRemote(); } catch (_) { diff --git a/mobile/test/domain/services/sync_linked_album_service_test.dart b/mobile/test/domain/services/sync_linked_album_service_test.dart new file mode 100644 index 0000000000..37d5629d5d --- /dev/null +++ b/mobile/test/domain/services/sync_linked_album_service_test.dart @@ -0,0 +1,186 @@ +import 'package:drift/drift.dart' as drift; +import 'package:drift/native.dart'; +import 'package:flutter/foundation.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/domain/models/album/album.model.dart'; +import 'package:immich_mobile/domain/models/album/local_album.model.dart'; +import 'package:immich_mobile/domain/models/store.model.dart'; +import 'package:immich_mobile/domain/services/store.service.dart'; +import 'package:immich_mobile/domain/services/sync_linked_album.service.dart'; +import 'package:immich_mobile/entities/store.entity.dart'; +import 'package:immich_mobile/infrastructure/repositories/db.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/store.repository.dart'; +import 'package:immich_mobile/repositories/drift_album_api_repository.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:openapi/api.dart'; + +import '../../fixtures/album.stub.dart'; +import '../../fixtures/user.stub.dart'; +import '../../infrastructure/repository.mock.dart'; + +RemoteAlbum _remoteAlbumFor(LocalAlbum local, {required String id}) => RemoteAlbum( + id: id, + name: local.name, + ownerId: UserStub.admin.id, + ownerName: UserStub.admin.name, + description: '', + createdAt: DateTime(2024), + updatedAt: DateTime(2024), + isActivityEnabled: true, + order: AlbumAssetOrder.desc, + assetCount: 0, + isShared: false, +); + +LocalAlbum _localAlbum({required String id, required String name, String? linkedRemoteAlbumId}) => LocalAlbum( + id: id, + name: name, + updatedAt: DateTime(2024), + assetCount: 5, + backupSelection: BackupSelection.selected, + isIosSharedAlbum: false, + linkedRemoteAlbumId: linkedRemoteAlbumId, +); + +void main() { + late SyncLinkedAlbumService sut; + late MockLocalAlbumRepository mockLocalAlbumRepo; + late MockRemoteAlbumRepository mockRemoteAlbumRepo; + late MockDriftAlbumApiRepository mockAlbumApiRepo; + late Drift db; + + setUpAll(() async { + TestWidgetsFlutterBinding.ensureInitialized(); + debugDefaultTargetPlatformOverride = TargetPlatform.android; + db = Drift(drift.DatabaseConnection(NativeDatabase.memory(), closeStreamsSynchronously: true)); + await StoreService.init(storeRepository: DriftStoreRepository(db)); + await Store.put(StoreKey.currentUser, UserStub.admin); + registerFallbackValue(LocalAlbumStub.recent); + registerFallbackValue(UserStub.admin); + registerFallbackValue( + RemoteAlbum( + id: 'fallback', + name: 'fallback', + ownerId: 'u', + ownerName: 'u', + description: '', + createdAt: DateTime(2024), + updatedAt: DateTime(2024), + isActivityEnabled: true, + order: AlbumAssetOrder.desc, + assetCount: 0, + isShared: false, + ), + ); + }); + + tearDownAll(() async { + debugDefaultTargetPlatformOverride = null; + await Store.clear(); + await db.close(); + }); + + setUp(() { + mockLocalAlbumRepo = MockLocalAlbumRepository(); + mockRemoteAlbumRepo = MockRemoteAlbumRepository(); + mockAlbumApiRepo = MockDriftAlbumApiRepository(); + + sut = SyncLinkedAlbumService(mockLocalAlbumRepo, mockRemoteAlbumRepo, mockAlbumApiRepo, StoreService.I); + + when(() => mockLocalAlbumRepo.linkRemoteAlbum(any(), any())).thenAnswer((_) async {}); + when(() => mockLocalAlbumRepo.unlinkRemoteAlbum(any())).thenAnswer((_) async {}); + when(() => mockRemoteAlbumRepo.deleteAlbum(any())).thenAnswer((_) async {}); + when(() => mockRemoteAlbumRepo.create(any(), any())).thenAnswer((_) async {}); + }); + + group('manageLinkedAlbums', () { + test('soft-fails when server fetch throws, no destructive writes', () async { + final local = _localAlbum(id: 'l1', name: 'Movies', linkedRemoteAlbumId: 'stale'); + when(() => mockAlbumApiRepo.getAllOwned(any())).thenThrow(ApiException(503, 'down')); + + await sut.manageLinkedAlbums([local], UserStub.admin.id); + + verifyNever(() => mockRemoteAlbumRepo.deleteAlbum(any())); + verifyNever(() => mockLocalAlbumRepo.linkRemoteAlbum(any(), any())); + verifyNever(() => mockAlbumApiRepo.createDriftAlbum(any(), any(), assetIds: any(named: 'assetIds'))); + }); + + test('no-op when linked album still exists on server', () async { + final local = _localAlbum(id: 'l1', name: 'Movies', linkedRemoteAlbumId: 'r1'); + final remote = _remoteAlbumFor(local, id: 'r1'); + when(() => mockAlbumApiRepo.getAllOwned(any())).thenAnswer((_) async => [remote]); + + await sut.manageLinkedAlbums([local], UserStub.admin.id); + + verifyNever(() => mockRemoteAlbumRepo.deleteAlbum(any())); + verifyNever(() => mockLocalAlbumRepo.linkRemoteAlbum(any(), any())); + verifyNever(() => mockAlbumApiRepo.createDriftAlbum(any(), any(), assetIds: any(named: 'assetIds'))); + }); + + test('prunes stale link when server no longer has the album', () async { + final local = _localAlbum(id: 'l1', name: 'Movies', linkedRemoteAlbumId: 'stale-id'); + when(() => mockAlbumApiRepo.getAllOwned(any())).thenAnswer((_) async => []); + when( + () => mockAlbumApiRepo.createDriftAlbum(any(), any(), assetIds: any(named: 'assetIds')), + ).thenAnswer((_) async => _remoteAlbumFor(local, id: 'new-id')); + + await sut.manageLinkedAlbums([local], UserStub.admin.id); + + verify(() => mockRemoteAlbumRepo.deleteAlbum('stale-id')).called(1); + verify(() => mockAlbumApiRepo.createDriftAlbum('Movies', UserStub.admin, assetIds: [])).called(1); + }); + + test('links to existing server album by name when unlinked', () async { + final local = _localAlbum(id: 'l1', name: 'Movies'); + final existing = _remoteAlbumFor(local, id: 'r-existing'); + when(() => mockAlbumApiRepo.getAllOwned(any())).thenAnswer((_) async => [existing]); + when(() => mockRemoteAlbumRepo.get('r-existing')).thenAnswer((_) async => null); + + await sut.manageLinkedAlbums([local], UserStub.admin.id); + + verify(() => mockRemoteAlbumRepo.create(existing, [])).called(1); + verify(() => mockLocalAlbumRepo.linkRemoteAlbum('l1', 'r-existing')).called(1); + verifyNever(() => mockAlbumApiRepo.createDriftAlbum(any(), any(), assetIds: any(named: 'assetIds'))); + }); + + test('creates a new remote album when no match on server', () async { + final local = _localAlbum(id: 'l1', name: 'Movies'); + final created = _remoteAlbumFor(local, id: 'r-new'); + when(() => mockAlbumApiRepo.getAllOwned(any())).thenAnswer((_) async => []); + when( + () => mockAlbumApiRepo.createDriftAlbum(any(), any(), assetIds: any(named: 'assetIds')), + ).thenAnswer((_) async => created); + + await sut.manageLinkedAlbums([local], UserStub.admin.id); + + verify(() => mockAlbumApiRepo.createDriftAlbum('Movies', UserStub.admin, assetIds: [])).called(1); + verify(() => mockRemoteAlbumRepo.create(created, [])).called(1); + verify(() => mockLocalAlbumRepo.linkRemoteAlbum('l1', 'r-new')).called(1); + }); + }); + + group('syncLinkedAlbums', () { + test('prunes cache row when addAssets throws RemoteAlbumNotFoundException', () async { + final local = _localAlbum(id: 'l1', name: 'Movies', linkedRemoteAlbumId: 'r-stale'); + final remote = _remoteAlbumFor(local, id: 'r-stale'); + when(() => mockLocalAlbumRepo.getBackupAlbums()).thenAnswer((_) async => [local]); + when(() => mockRemoteAlbumRepo.get('r-stale')).thenAnswer((_) async => remote); + when(() => mockRemoteAlbumRepo.getLinkedAssetIds(any(), any(), any())).thenAnswer((_) async => ['a1']); + when(() => mockAlbumApiRepo.addAssets('r-stale', any())).thenThrow(const RemoteAlbumNotFoundException('r-stale')); + + await sut.syncLinkedAlbums(UserStub.admin.id); + + verify(() => mockRemoteAlbumRepo.deleteAlbum('r-stale')).called(1); + }); + + test('skips albums with null linked id without server calls', () async { + final local = _localAlbum(id: 'l1', name: 'Movies'); + when(() => mockLocalAlbumRepo.getBackupAlbums()).thenAnswer((_) async => [local]); + + await sut.syncLinkedAlbums(UserStub.admin.id); + + verifyNever(() => mockAlbumApiRepo.addAssets(any(), any())); + verifyNever(() => mockRemoteAlbumRepo.deleteAlbum(any())); + }); + }); +}