fix: keep cache info of removed files when another file cannot be deleted by junsworth · Pull Request #541 · Baseflow/flutter_cache_manager

junsworth · 2026-10-22T11:52:22Z

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Bug fix.

⤵️ What is the current behavior?

If a cached file can't be deleted (for example a file locked on Windows), Future.wait throws and deleteAll is skipped for the whole batch. getOldObjects and getObjectsOverCapacity return at most 100 rows each, so one file that stays undeletable makes every cleanup reselect the same rows and stop there. emptyCache has the same all-or-nothing behavior.

🆕 What is the new behavior (if this is a feature change)?

_removeCachedFile only marks a file for cache-info deletion once the file is gone or confirmed missing. Cleanup, emptyCache and removeCachedFile then delete the cache info of every removed file and keep only the failed file's cache info, so the next cleanup retries it. The file error is still reported, and if deleteAll also fails that is logged instead of replacing it.

💥 Does this PR introduce a breaking change?

No.

🐛 Recommendations for testing

New tests in cache_store_test.dart cover a failed removal during cleanup, a failed removal in emptyCache, and the file error not being masked when deleteAll also fails. The ordering test ("should not remove cache info before the cached file is removed") already passes on main since #542, I kept it as a regression guard.

flutter test, dart format --set-exit-if-changed . and flutter analyze pass locally on Flutter 3.47.4. I haven't built the example apps locally, CI does that.

📝 Links to relevant issues/docs

Follows up on #542, which already fixed the ordering of file removal and cache-info deletion. Related: #535.

🤔 Checklist before submitting

All projects build
Follows style guide lines (code style guide)
Relevant documentation was updated
Rebased onto current main
Version bumped and dated CHANGELOG entry added

…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.

junsworth · 2026-10-28T11:14:42Z

Hey @rickdijk ,

Please could you confirm that the following pull request #542 fixes the issue presented here in this pull request? I believe it does so thank you 🙏

I do have data to back up the orphaned files scenario that we are seeing and before closing this pull request I would like to confirm that this issue is fixed please.

Please could you confirm this?

Thanks

rickdijk

Thanks for the detailed write-up, @junsworth. To answer your question first: yes, #542 (released in 3.4.6) already fixes the ordering problem this PR describes. On current main, _cleanupCache awaits Future.wait(futures) before provider.deleteAll(toRemove), so if the app is killed between the two steps you get a row without a file, not a file without a row. Your branch is based on that commit.

Does it fix something? Not the headline bug, since that is already fixed. There is still one real improvement, though. On main, if a single file can't be deleted (for example a file locked on Windows), Future.wait throws and deleteAll is skipped for the whole batch. getOldObjects and getObjectsOverCapacity return at most 100 rows each, so one file that stays undeletable makes every cleanup reselect the same rows and stop there. Keeping only the failed row and deleting the rest fixes that.

Does it add value? Yes, but only the partial-failure part. The title, description and CHANGELOG should describe that change rather than the ordering fix.

Tests: The second test (should keep cache info of a file that cannot be removed) covers the actual change and fails on main. On main it fails because deleteAll is never called, not because the call includes id 2. The first test already passes on main since #542. It's fine to keep as a regression guard, but it doesn't verify this PR.

Breaking change? No. There are no API changes, and the only behavior change is in the failure path.

Contribution guidelines: The template is filled in, there's one package, and the version is bumped with a dated entry. Some of the text is out of date: the description still says both PRs bump to 3.4.6 and that both tests fail on current main, and the CHANGELOG entry credits 3.4.7 with a fix that 3.4.6 already shipped. "All projects build" is left unchecked, so CI will settle that.

I've left inline comments on the details.

rickdijk · 2026-10-29T09:25:16Z


-    await Future.wait(futures);
-    await provider.deleteAll(toRemove);
+    try {


This ordering is already on main via #542: the code before this PR awaited Future.wait(futures) and only then called deleteAll. The new behavior in this hunk is the partial-failure handling: the failed row is kept and every other row is still deleted. On main, one failure skips deleteAll for the whole batch. Could you reword the comment and the PR around that?

Agreed, reworded the PR and removed that comment. The partial-failure handling is the only thing this adds now.

rickdijk · 2026-10-29T09:25:16Z

+      // 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);


When Future.wait has already thrown and deleteAll then throws too (the SQLite error 14 case from #535), the deleteAll exception replaces the file-deletion error. _scheduleCleanup then logs only the DB error. It's minor, but you might want to catch and log the deleteAll failure here, or rethrow the original error.

Good catch. The deleteAll failure is now logged and the original file error is rethrown, so _scheduleCleanup logs the more useful one. I added a test for it. If deleteAll is the only thing that fails, it still propagates as before.

rickdijk · 2026-10-29T09:25:16Z

+    }
  }

  Future<void> emptyCache() async {


emptyCache still uses the same all-or-nothing pattern (Future.wait and then deleteAll), so one undeletable file there still skips deleteAll for every row. Could the fix live in _removeCachedFile instead, adding the id to toRemove only after the file is gone or confirmed missing (with a separate set for the in-flight dedup)? That would cover both callers and remove the need for the remove closure, the rethrow and the finally.

That's a nicer place for it, thanks. The id now only goes into the list after the file is deleted or already missing, and a separate started set does the dedup. One thing I found along the way: Future.wait still throws on a failed removal, so there's still a small shared helper that waits, deletes the cache info of the successful ones and then rethrows. It's used by _cleanupCache, emptyCache and removeCachedFile. I also added an emptyCache test for the partial failure.

rickdijk · 2026-10-29T09:25:16Z

@@ -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 already shipped this fix (#542), so this entry would suggest 3.4.6 still had the bug. Suggested wording for what actually changes:

Suggested change

* 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.

* Keeps cache cleanup going when a cached file cannot be deleted: the cache info of the other removed files is still deleted, and only the failed file's cache info is kept so the next cleanup retries it.

Good point, updated with your wording.

rickdijk · 2026-10-29T09:25:17Z

    });

+    test(
+      'Store should not remove cache info before the cached file is removed',


Since #542 this test also passes on current main, because _cleanupCache there already awaits the removals before deleteAll. So it guards #542 rather than this PR. That's fine to keep, but the description saying both tests fail on main is out of date.

You're right. I've kept it as a regression guard and fixed the description, which no longer says both tests fail on main.

rickdijk · 2026-10-29T09:25:17Z

+        expect(log, contains('Cannot delete'));
+        verify(
+          config.mockRepo.deleteAll(
+            argThat(allOf(contains(okObject.id), isNot(contains(2)))),


Nit: use brokenObject.id instead of the literal 2, to match okObject.id in the same matcher.

Suggested change

argThat(allOf(contains(okObject.id), isNot(contains(2)))),

argThat(allOf(contains(okObject.id), isNot(contains(brokenObject.id)))),

Done, thanks.

Move the partial-failure handling into _removeCachedFile so cleanup, emptyCache and removeCachedFile all delete the cache info of removed files and keep only the failed one. A failing deleteAll is logged instead of replacing the file removal error.

junsworth · 2026-10-30T08:58:05Z

Thanks a lot for the thorough review, @rickdijk, and for confirming #542 covers the ordering problem. That makes sense, my branch was just based on a point before it landed. I've reworked this PR around the partial-failure part you described: the title, description and CHANGELOG now say that, and I used your suggested CHANGELOG wording. I've also moved the fix into _removeCachedFile as you suggested, so emptyCache and removeCachedFile get it too. Details are in the inline replies.

junsworth force-pushed the fix/await-file-removal-before-cache-info branch from 3d58574 to 519ad8b Compare October 28, 2026 11:02

rickdijk reviewed Oct 29, 2026

View reviewed changes

junsworth changed the title ~~fix: wait for cached files to be removed before deleting their cache info~~ fix: keep cache info of removed files when another file cannot be deleted Oct 30, 2026

		@@ -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.

	* 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.
	* Keeps cache cleanup going when a cached file cannot be deleted: the cache info of the other removed files is still deleted, and only the failed file's cache info is kept so the next cleanup retries it.

	argThat(allOf(contains(okObject.id), isNot(contains(2)))),
	argThat(allOf(contains(okObject.id), isNot(contains(brokenObject.id)))),

Conversation

junsworth commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

⤵️ What is the current behavior?

🆕 What is the new behavior (if this is a feature change)?

💥 Does this PR introduce a breaking change?

🐛 Recommendations for testing

📝 Links to relevant issues/docs

🤔 Checklist before submitting

Uh oh!

junsworth commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickdijk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junsworth commented Oct 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

junsworth commented Oct 22, 2026 •

edited

Loading

junsworth commented Oct 28, 2026 •

edited

Loading