Await store.putFile so cache info is persisted #492 - #518
Conversation
Sorry, something went wrong.
rickdijk
left a comment
There was a problem hiding this comment.
Please address my comments so we can proceed with this fix
Sorry, something went wrong.
| }); | ||
| await _store.putFile(newCacheObject); | ||
| if (newCacheObject.relativePath != oldCacheObject.relativePath) { | ||
| await _removeOldFile(oldCacheObject.relativePath); |
There was a problem hiding this comment.
This function _removeOldFile lacks error handling, can you add that?
This would probably do:
try {
if (await file.exists()) {
await file.delete();
}
} on FileSystemException {
// Already deleted (see #184) or not deletable. The cache info no longer
// points at this path, so there is nothing to recover here.
}
Sorry, something went wrong.
| expect(arg.url, fileUrl); | ||
| }); | ||
|
|
||
| test('putFile waits for store persist before returning', () async { |
There was a problem hiding this comment.
May I suggest the following, since we try to test persistence without relying on timers:
test('putFile waits for store persist before returning', () async {
final persisted = Completer<void>();
final store = MockCacheStore();
when(store.putFile(any)).thenAnswer((_) => persisted.future);
final cacheManager = TestCacheManager(createTestConfig(), store: store);
var returned = false;
final put = cacheManager.putFile('baseflow.com/test', Uint8List(8))
..whenComplete(() => returned = true);
await pumpEventQueue();
expect(returned, isFalse, reason: 'putFile returned before the store persisted');
persisted.complete();
await put;
});
Sorry, something went wrong.
| await cacheManager.putFile('baseflow.com/test', Uint8List(8)); | ||
| expect(persistDone, isTrue); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Please also add a test that actually tests if the file removal works:
test('removeFile deletes the entry right after putFile', () async {
final repo = JsonCacheInfoRepository.withFile(
await JsonRepoHelpers.createDatabaseFile(),
);
final config = Config(
'test',
fileSystem: TestFileSystem(),
repo: repo,
fileService: MockFileService(),
);
final cacheManager = TestCacheManager(config);
const url = 'baseflow.com/test';
final file = await cacheManager.putFile(url, Uint8List(8), fileExtension: 'jpg');
await cacheManager.removeFile(url);
await pumpEventQueue();
expect(await repo.get(url), isNull);
expect(await file.exists(), isFalse);
});
Sorry, something went wrong.
| ); | ||
| }); | ||
|
|
||
| var webHelper = WebHelper(store, fileService); |
There was a problem hiding this comment.
| var webHelper = WebHelper(store, fileService); | |
| final webHelper = WebHelper(store, fileService); |
Sorry, something went wrong.
| var config = createTestConfig(); | ||
| var store = _createStore(config); | ||
| when(store.putFile(any)).thenAnswer((_) async { | ||
| await Future<void>.delayed(const Duration(milliseconds: 40)); |
There was a problem hiding this comment.
Also here prevent using a timer and go with the Completer instead.
Sorry, something went wrong.
|
Added the try/catch on _removeOldFile, switched the persist tests to Completers, and added the removeFile-after-putFile case. |
Sorry, something went wrong.
putFile, putFileStream, and downloads started persist without waiting. After those calls returned, CacheObject.id could still be null, so removeFile skipped the entry and a process exit could lose the info. Fixes Baseflow#492
Catch FileSystemException when deleting a stale cache file.
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Bug fix
⤵️ What is the current behavior?
putFile, putFileStream, and WebHelper start store.putFile and return without waiting. The file is on disk, but the cache-info row (and its id) may not be written yet.
That matches #492: removeFile after getFileStream can no-op because CacheObject.id is still null, and a crash right after download can drop the entry.
🆕 What is the new behavior (if this is a feature change)?
Those paths now await persist. After they complete, the store has the object and an id, so a follow-up removeFile actually deletes it.
💥 Does this PR introduce a breaking change?
No. Callers already treat these as Futures. They just wait a bit longer for the existing database write.
🐛 Recommendations for testing
flutter test in flutter_cache_manager. New cases delay store.putFile and fail if the caller returns early.
📝 Links to relevant issues/docs
Fixes #492
🤔 Checklist before submitting