Improve podcast artwork caching#5549
Conversation
Give the Coil artwork disk cache a fixed 250MB size instead of the default 2% of free disk space (10MB floor), and add a daily worker that re-fetches subscribed podcast artwork evicted from the disk cache so covers still display when the network or CDN is unavailable. PCDROID-318
|
Claude finished @joashrajin's task in 3m 12s —— View job Code Review — Improve podcast artwork caching
Nice, well-scoped fix with a clear root-cause writeup. The approach (deterministic cache size + a self-healing prefetch worker) is sound and the implementation is consistent with the existing I left 4 inline comments. Summary below. Correctness / robustness
Design / trade-offs
Minor / nits
Non-blocking observations
Overall: approvable after considering the |
|
Claude finished @joashrajin's task in 2m 39s —— View job PR Review: Improve podcast artwork caching
Nice, focused change — clear problem statement, and the code fits the existing conventions well (mirrors ✅ What's correct
🔧 Suggestions (in priority order)
💭 Minor notes
Overall this is a sound fix. The |
Generated by 🚫 Danger |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Coil's default size is 2% of the free disk space with a 10MB floor, which on | ||
| // devices low on storage evicts artwork almost immediately. | ||
| .maxSizeBytes(ARTWORK_DISK_CACHE_SIZE_BYTES) | ||
| .build() | ||
| } | ||
| .build() | ||
| } | ||
|
|
||
| private companion object { | ||
| const val ARTWORK_DISK_CACHE_SIZE_BYTES = 250L * 1024 * 1024 |
There was a problem hiding this comment.
Fixed 250 MB cache may work against the exact scenario this PR targets. The root-cause analysis says the problem is devices low on storage, but a hard maxSizeBytes(250MB) removes the free-space-relative safety valve that Coil's default provides. On a device with, say, 150 MB free, Coil will still grow this cache toward 250 MB, contributing to storage pressure — and the OS is then more likely to trim the whole cache dir anyway (which is the eviction you're trying to avoid).
A couple of things to weigh:
- Consider clamping to a fraction of free space rather than an unconditional constant, e.g. keep a generous floor but cap at available space:
.maxSizeBytes(ARTWORK_DISK_CACHE_SIZE_BYTES) // or coerce against StatFs free bytes
- This
ImageLoaderis the app-wide one, so 250 MB is shared by all images (episode/user artwork, discover, avatars…), not just podcast covers. Worth confirming 250 MB is the intended total budget, and that the number is justified (rough math: ~250 podcasts × 3 sizes × ~200 KB ≈ 150 MB).
Not blocking, but a comment documenting why 250 MB and the low-storage trade-off would help the next reader.
| continue | ||
| } | ||
| val request = ImageRequest.Builder(applicationContext) | ||
| .data(url) | ||
| .memoryCachePolicy(CachePolicy.DISABLED) | ||
| .build() | ||
| val result = imageLoader.execute(request) | ||
| if (result is ErrorResult) { | ||
| Timber.i("Could not prefetch artwork from $url. ${result.throwable.message}") | ||
| } else { | ||
| fetched++ | ||
| } | ||
| } | ||
| } | ||
| Timber.i("Prefetched $fetched missing podcast artwork images.") | ||
| return Result.success() |
There was a problem hiding this comment.
The worker returns Result.success() even when every fetch failed. In the precise outage scenario this PR is built for (CDN down but device online), the network constraint is satisfied, all execute() calls come back as ErrorResult, and the worker reports success — so nothing re-runs until the next daily tick, up to ~24h later.
Consider tracking failures and returning Result.retry() when some/all fetches failed, so the cache self-heals shortly after the CDN recovers rather than a day later:
var failed = 0
// ... in the loop: if (result is ErrorResult) { failed++; Timber.i(...) } else { fetched++ }
Timber.i("Prefetched $fetched missing podcast artwork images ($failed failed).")
return if (failed > 0) Result.retry() else Result.success()Also worth wrapping the body in a try/catch → Result.retry(): podcastManager.findSubscribedNoOrder() and openSnapshot() do I/O and can throw; an uncaught exception marks the run as failed with no controlled retry policy.
| continue | ||
| } | ||
| val request = ImageRequest.Builder(applicationContext) | ||
| .data(url) |
There was a problem hiding this comment.
Load-bearing assumption: openSnapshot(url) matches the disk-cache key Coil writes. This is correct today because the ImageLoader in UiModule registers no custom Keyer/diskCacheKey, so Coil defaults the disk-cache key to the data string (the URL). But it's the single point the whole worker hinges on, and it's untested — if a keyer is ever added, isCached becomes permanently false and the worker silently re-fetches everything every day. A one-line comment noting this dependency (and ideally the same PocketCastsImageRequestFactory path being used for reads) would protect it.
Minor: diskCache.openSnapshot(url)?.use { true } ?: false is fine, but openSnapshot(url)?.use { } != null reads a touch more clearly.
| .memoryCachePolicy(CachePolicy.DISABLED) | ||
| .build() | ||
| val result = imageLoader.execute(request) | ||
| if (result is ErrorResult) { | ||
| Timber.i("Could not prefetch artwork from $url. ${result.throwable.message}") | ||
| } else { | ||
| fetched++ | ||
| } | ||
| } | ||
| } | ||
| Timber.i("Prefetched $fetched missing podcast artwork images.") |
There was a problem hiding this comment.
Duplication with SubscribeManager.cacheArtworkRxCompletable. This builds essentially the same ImageRequest (.data(url).memoryCachePolicy(DISABLED) + execute + ErrorResult logging) that already exists in SubscribeManager. Consider extracting a single shared prefetch helper (e.g. in the repositories layer alongside PodcastImage) so the caching contract lives in one place and can't drift between subscribe-time and worker-time.
Perf nit (not blocking for a daily job): the per-podcast × per-size loop runs fully sequentially. For a large library that's hundreds of serialized execute() calls; a bounded concurrent fan-out (e.g. coroutineScope + a small semaphore) would finish faster and shorten the wakelock. Also, the openSnapshot disk reads run on Dispatchers.Default — fine, but Dispatchers.IO is the more correct home for that I/O.
| if (result is ErrorResult) { | ||
| Timber.i("Could not prefetch artwork from $url. ${result.throwable.message}") | ||
| } else { | ||
| fetched++ | ||
| } | ||
| } | ||
| } | ||
| Timber.i("Prefetched $fetched missing podcast artwork images.") | ||
| return Result.success() |
There was a problem hiding this comment.
Consider Result.retry() when fetches fail. This is the most important point given the feature's goal.
During the exact scenario this worker targets — a CDN/network outage — every execute returns ErrorResult, the failures are logged, and the worker still returns Result.success(). Because it's a periodic worker, the missing artwork then won't be re-fetched until the next daily run, so the cache won't self-heal promptly once the CDN recovers (it could be up to ~24h later).
Consider tracking whether any fetch failed and returning Result.retry() in that case, so WorkManager retries with exponential backoff (set via setBackoffCriteria) minutes after connectivity/CDN is restored rather than a day later.
var failed = 0
...
if (result is ErrorResult) {
failed++
Timber.i("Could not prefetch artwork from $url. ${result.throwable.message}")
} else {
fetched++
}
...
Timber.i("Prefetched $fetched missing podcast artwork images.")
return if (failed > 0) Result.retry() else Result.success()
| for (podcast in podcastManager.findSubscribedNoOrder()) { | ||
| for (url in PodcastImage.getArtworkUrls(uuid = podcast.uuid, isWearOS = isWearOs)) { | ||
| val isCached = diskCache.openSnapshot(url)?.use { true } ?: false | ||
| if (isCached) { | ||
| continue | ||
| } | ||
| val request = ImageRequest.Builder(applicationContext) | ||
| .data(url) | ||
| .memoryCachePolicy(CachePolicy.DISABLED) | ||
| .build() | ||
| val result = imageLoader.execute(request) | ||
| if (result is ErrorResult) { | ||
| Timber.i("Could not prefetch artwork from $url. ${result.throwable.message}") | ||
| } else { | ||
| fetched++ | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Sequential fetching may be slow for large libraries. A user with, say, 300 subscriptions triggers up to 900 sequential openSnapshot disk reads + network fetches, one at a time. It's a background worker so latency is tolerable, but on the first run after a cache wipe this is a long serial chain. Consider bounded parallelism (e.g. chunked() + coroutineScope { async {} }, or a Semaphore) to cut wall-clock time.
Minor: diskCache.openSnapshot(url) is blocking disk I/O and CoroutineWorker.doWork() runs on Dispatchers.Default by default — wrapping the loop body in withContext(Dispatchers.IO) would be more correct for the I/O-bound work.
| downloadStatusObserver.monitorDownloadStatus() | ||
| userManager.beginMonitoringAccountManager(playbackManager) | ||
| CuratedPodcastsSyncWorker.enqueuePeriodicWork(this) | ||
| PrefetchArtworkWorker.enqueuePeriodicWork(this) |
There was a problem hiding this comment.
The worker is only enqueued here in the mobile app. The worker itself has an isWearOs branch (PodcastImage.getArtworkUrls(..., isWearOS = ...)), and the 250MB cache change lives in the shared ui UiModule, so Wear/Automotive get the larger cache but no self-healing worker. If that's intentional (e.g. Wear artwork persistence isn't a concern), fine — but worth confirming, since the isWearOs handling in the worker reads as if Wear support was intended. On the mobile app that branch is always false.
Address review feedback: return Result.retry() with exponential backoff when any artwork fetch fails or the worker throws, run the prefetch loop on Dispatchers.IO, and document the disk cache key assumption and the 250MB cache size rationale.
|
Claude finished @joashrajin's task in 1m 8s —— View job Re-review after
|
2 participants
Description
Improves podcast artwork caching so covers still display when the network or the artwork CDN is unavailable (as happened during the Cloudflare outage, where users reported all thumbnails disappearing).
Root cause: the Coil disk cache for artwork uses the library's default size — 2% of the free disk space with a 10MB floor. On devices low on storage the artwork cache is tiny and constantly LRU-evicts. Artwork is prefetched once at subscribe time (
SubscribeManager.cacheArtworkRxCompletable), but once an entry is evicted nothing restores it until a successful network load — so during an outage the app shows placeholder tiles.Changes:
UiModule: give the artworkDiskCachea fixed 250MBmaxSizeBytesinstead of the free-space-relative default, so cache capacity is deterministic regardless of device storage pressure.PrefetchArtworkWorker(new): a daily WorkManager job (network-connected constraint) that checks each subscribed podcast's artwork URLs against the disk cache and re-fetches only the missing ones, so the cache self-heals after eviction while the device is online.PocketCastsApplication: enqueue the periodic worker at startup, alongsideCuratedPodcastsSyncWorker.Fixes PCDROID-318
Testing Instructions
adb shell run-as au.com.shiftyjelly.pocketcasts.debug rm -r cache/ImageCache(or clear the app cache from system settings) to simulate the disk cache being evicted, then force-stop and relaunch the app while online.adb shell am broadcastalternatives aside, the simplest isadb shell cmd jobscheduleror WorkManager's test hook — or just wait for the dailyPrefetchArtworkWorkerPeriodicrun.Prefetched N missing podcast artwork images.(Timber) and re-running it logsPrefetched 0 ...(everything cached).Screenshots or Screencast
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml— n/a, no strings addedNo unit test added: the new worker is glue around Coil's
ImageLoader/DiskCacheand WorkManager with no seam to test without instrumenting Coil itself.I have tested any UI changes...