From 519ad8b1ccb9f08209df44d19ad36ddcb5794dfa Mon Sep 17 00:00:00 2001 From: Jonathan Unsworth <17317065+junsworth@users.noreply.github.com> Date: Thu, 22 Oct 2026 12:47:34 +0100 Subject: [PATCH] fix: wait for cached files to be removed before deleting their cache info The cleanup started removing files without waiting and then deleted the cache info right away, so the cache info could be gone while the files were still on disk. If the app was killed in between, those files were left with nothing referencing them. The cleanup now waits for the file removals first. The cache info of a file that could not be removed is kept, so the next cleanup retries it. --- flutter_cache_manager/CHANGELOG.md | 4 + .../lib/src/cache_store.dart | 28 +++- flutter_cache_manager/pubspec.yaml | 2 +- .../test/cache_store_test.dart | 137 ++++++++++++++++++ 4 files changed, 165 insertions(+), 6 deletions(-) diff --git a/flutter_cache_manager/CHANGELOG.md b/flutter_cache_manager/CHANGELOG.md index 275c791b..b83df808 100644 --- a/flutter_cache_manager/CHANGELOG.md +++ b/flutter_cache_manager/CHANGELOG.md @@ -1,3 +1,7 @@ +## [3.4.7] - 2026-10-28 + +* Fixes the cache cleanup deleting the cache info before the cached files were removed. When the app was killed in between, files stayed on disk with nothing referencing them. The cache info of a file that could not be removed is now kept, so the next cleanup retries it. + ## [3.4.6] - 2026-10-27 * Catches unhandled asynchronous errors when updating cache-hit metadata and during scheduled cleanup in `CacheStore` ([#535](https://github.com/Baseflow/flutter_cache_manager/issues/535)). diff --git a/flutter_cache_manager/lib/src/cache_store.dart b/flutter_cache_manager/lib/src/cache_store.dart index eaf4d189..dd360c35 100644 --- a/flutter_cache_manager/lib/src/cache_store.dart +++ b/flutter_cache_manager/lib/src/cache_store.dart @@ -201,19 +201,37 @@ class CacheStore { final toRemove = []; final provider = await _cacheInfoRepository; + final removals = >[]; + + Future remove(CacheObject cacheObject) async { + try { + await _removeCachedFile(cacheObject, toRemove); + } catch (_) { + // Keep the cache info of a file that could not be removed, so the next + // cleanup tries again instead of leaving the file behind. + toRemove.remove(cacheObject.id); + rethrow; + } + } + final overCapacity = await provider.getObjectsOverCapacity(_capacity); - final futures = []; for (final cacheObject in overCapacity) { - futures.add(_removeCachedFile(cacheObject, toRemove)); + removals.add(remove(cacheObject)); } final oldObjects = await provider.getOldObjects(_maxAge); for (final cacheObject in oldObjects) { - futures.add(_removeCachedFile(cacheObject, toRemove)); + removals.add(remove(cacheObject)); } - await Future.wait(futures); - await provider.deleteAll(toRemove); + try { + await Future.wait(removals); + } finally { + // Only delete the cache info once the files are gone. When the app is + // killed in between, a cache info without a file is cleaned up on the next + // read, but a file without a cache info would never be found again. + await provider.deleteAll(toRemove); + } } Future emptyCache() async { diff --git a/flutter_cache_manager/pubspec.yaml b/flutter_cache_manager/pubspec.yaml index 8456c4b5..c3d22cd7 100644 --- a/flutter_cache_manager/pubspec.yaml +++ b/flutter_cache_manager/pubspec.yaml @@ -1,6 +1,6 @@ name: flutter_cache_manager description: Generic cache manager for flutter. Saves web files on the storages of the device and saves the cache info using sqflite. -version: 3.4.6 +version: 3.4.7 homepage: https://github.com/Baseflow/flutter_cache_manager topics: - cache diff --git a/flutter_cache_manager/test/cache_store_test.dart b/flutter_cache_manager/test/cache_store_test.dart index 91bdf864..fb653d2f 100644 --- a/flutter_cache_manager/test/cache_store_test.dart +++ b/flutter_cache_manager/test/cache_store_test.dart @@ -6,6 +6,8 @@ import 'package:file/memory.dart'; import 'package:flutter_cache_manager/src/cache_store.dart'; import 'package:flutter_cache_manager/src/logger.dart'; import 'package:flutter_cache_manager/src/storage/cache_object.dart'; +import 'package:flutter_cache_manager/src/storage/file_system/file_system.dart' + as cache_file_system; import 'package:flutter_test/flutter_test.dart'; import 'package:mockito/mockito.dart'; @@ -464,6 +466,103 @@ void main() { ).called(1); }); + test( + 'Store should not remove cache info before the cached file is removed', + () async { + var config = createTestConfig(); + var store = CacheStore(config); + store.cleanupRunMinInterval = const Duration(milliseconds: 1); + var file = await config.returnsFile('testimage.png'); + + var cacheObject = CacheObject( + 'baseflow.com/test.png', + relativePath: 'testimage.png', + id: 1, + validTill: clock.now().add(const Duration(days: 7)), + ); + + when( + config.mockRepo.getObjectsOverCapacity(any), + ).thenAnswer((_) => Future.value([])); + when( + config.mockRepo.getOldObjects(any), + ).thenAnswer((_) => Future.value([cacheObject])); + when( + config.mockRepo.get('baseflow.com/test.png'), + ).thenAnswer((_) => Future.value(cacheObject)); + + expect(await store.getFile('baseflow.com/test.png'), isNotNull); + + // From here on, getting to the file takes a while, like a slow disk. + final fileAvailable = Completer(); + store.fileSystem = _DelayedFileSystem( + store.fileSystem, + fileAvailable.future, + ); + + // Give the cleanup time to run. It must not delete the cache info + // while the file is still on disk. + await Future.delayed(const Duration(milliseconds: 50)); + verifyNever(config.mockRepo.deleteAll(any)); + expect(file.existsSync(), isTrue); + + fileAvailable.complete(); + await untilCalled(config.mockRepo.deleteAll(any)); + + expect(file.existsSync(), isFalse); + verify( + config.mockRepo.deleteAll(argThat(contains(cacheObject.id))), + ).called(1); + }, + ); + + test( + 'Store should keep cache info of a file that cannot be removed', + () async { + final fileSystem = _FailingDeleteFileSystem('broken.png'); + final config = createTestConfig(); + final store = CacheStore(config); + store.cleanupRunMinInterval = const Duration(milliseconds: 1); + store.fileSystem = fileSystem; + await fileSystem.createFileWithData('ok.png'); + await fileSystem.createFileWithData('broken.png'); + + final okObject = CacheObject( + 'baseflow.com/ok.png', + relativePath: 'ok.png', + id: 1, + validTill: clock.now().add(const Duration(days: 7)), + ); + final brokenObject = CacheObject( + 'baseflow.com/broken.png', + relativePath: 'broken.png', + id: 2, + validTill: clock.now().add(const Duration(days: 7)), + ); + + when( + config.mockRepo.getObjectsOverCapacity(any), + ).thenAnswer((_) => Future.value([])); + when( + config.mockRepo.getOldObjects(any), + ).thenAnswer((_) => Future.value([okObject, brokenObject])); + when( + config.mockRepo.get('baseflow.com/ok.png'), + ).thenAnswer((_) => Future.value(okObject)); + + // The failed removal is still reported as a logged cleanup failure. + final logger = setUpTestLogger(); + expect(await store.getFile('baseflow.com/ok.png'), isNotNull); + final log = await logger.waitForLog('Failed to cleanup cache'); + expect(log, contains('Cannot delete')); + verify( + config.mockRepo.deleteAll( + argThat(allOf(contains(okObject.id), isNot(contains(2)))), + ), + ).called(1); + }, + ); + test('Store should recheck cache info when file is removed', () async { var config = createTestConfig(); var store = CacheStore(config); @@ -624,3 +723,41 @@ TestCacheLogger setUpTestLogger() { addTearDown(() => cacheLogger = originalLogger); return testLogger; } + +/// A file system that only hands out files once [_available] has completed. +class _DelayedFileSystem extends cache_file_system.FileSystem { + _DelayedFileSystem(this._inner, this._available); + + final cache_file_system.FileSystem _inner; + final Future _available; + + @override + Future createFile(String name) async { + await _available; + return _inner.createFile(name); + } +} + +/// A file system in which deleting the file called [undeletableName] fails. +class _FailingDeleteFileSystem extends cache_file_system.FileSystem { + _FailingDeleteFileSystem(String undeletableName) + : _fileSystem = MemoryFileSystem( + opHandle: (path, operation) { + if (operation == FileSystemOp.delete && + path.endsWith(undeletableName)) { + throw FileSystemException('Cannot delete', path); + } + }, + ); + + final MemoryFileSystem _fileSystem; + late final Directory _directory = _fileSystem.systemTempDirectory + .createTempSync('test'); + + @override + Future createFile(String name) async => _directory.childFile(name); + + Future createFileWithData(String name) async { + await (await createFile(name)).writeAsBytes([1, 3]); + } +}