diff --git a/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt b/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt index 918decff67d..43d148bb457 100644 --- a/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt +++ b/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt @@ -1523,15 +1523,15 @@ internal constructor( public fun clearCacheAndTemporaryFiles(context: Context): Call = CoroutineCall(clientScope) { logger.d { "[clearCacheAndTemporaryFiles] Clearing all cache and temporary files" } - // Clear video cache: in-place via any live cache in the process (keeps the SimpleCache - // alive so playback continues to work), or by deleting the directory when no live cache - // owns it. The registry is process-wide, so this covers caches from a prior ChatClient - // build even if the current client was built without a cacheConfig. - val videoCacheResult = if (VideoMediaCache.clearAll()) { - Result.Success(Unit) - } else { - fileManager.clearVideoCache(context) - } + // Clear the video cache for its directory atomically: if a live cache owns the + // directory, clear it in place (keeps the SimpleCache alive so playback continues to + // work); otherwise delete the directory from disk. Deciding and deleting under the + // registry lock prevents a concurrent client build from registering a new cache for the + // directory mid-delete, which would corrupt Media3's on-disk index and lock. + val videoCacheResult = VideoMediaCache.clearOrDelete( + cacheDir = fileManager.getVideoCache(context), + deleteOnDisk = { fileManager.clearVideoCache(context) }, + ) // Clear all cache directories val cacheResult = fileManager.clearAllCache(context) // Clear external (temporary) storage files - always run regardless of cache result diff --git a/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt b/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt index 449f1c5b231..4147ad13845 100644 --- a/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt +++ b/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt @@ -36,6 +36,9 @@ public data class StreamCacheConfig( * enabled, replaying or seeking within a previously watched video reuses cached byte ranges * instead of re-downloading from the CDN. * + * Cache entries are keyed by URL path only; query parameters are stripped. URLs that differ + * only in their query string share the same cache entry. + * * @param maxSizeBytes Soft cap on cache size; LRU eviction kicks in once exceeded. Files larger * than this cap are not effectively cached. Size [maxSizeBytes] to comfortably exceed the * largest expected video. diff --git a/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt b/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt index 90574907655..58ebf155fe3 100644 --- a/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt +++ b/stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt @@ -26,6 +26,7 @@ import androidx.media3.datasource.cache.SimpleCache import io.getstream.chat.android.client.cache.VideoCacheConfig import io.getstream.chat.android.core.internal.InternalStreamChatApi import io.getstream.log.taggedLogger +import io.getstream.result.Result import java.io.File /** @@ -111,31 +112,41 @@ public class VideoMediaCache private constructor( private val logger by taggedLogger(TAG) /** - * Clears every live [VideoMediaCache] in this process in place (see [clear]), keeping each - * [SimpleCache] and its directory lock alive. Returns `true` if at least one live cache was - * cleared, `false` if the registry was empty (no cache directory is owned in this process). + * Clears the video cache backing [cacheDir] atomically under the registry lock. * - * Callers use the return value to decide whether it is safe to delete the cache directory - * from disk: deleting a directory owned by a live [SimpleCache] corrupts Media3's on-disk - * index and lock. + * If a live [VideoMediaCache] owns [cacheDir], clears it in place (see [clear]) so the + * [SimpleCache] and its directory lock stay alive and playback keeps working. Otherwise runs + * [deleteOnDisk] to remove the directory from disk. The "is a cache live?" check and the + * delete happen under the same lock, so a concurrent [create] cannot register a new cache + * for [cacheDir] between them — which would otherwise let the delete corrupt Media3's + * on-disk index and lock. + * + * @param cacheDir Directory whose video cache should be cleared, used to look up any live + * cache that owns it. Must resolve to the same directory that [deleteOnDisk] removes. + * @param deleteOnDisk Deletes the cache directory from disk. Invoked only when no live cache + * owns [cacheDir]. */ - internal fun clearAll(): Boolean = synchronized(instances) { - instances.values.forEach { it.clear() } - instances.isNotEmpty() - } + internal fun clearOrDelete(cacheDir: File, deleteOnDisk: () -> Result): Result = + synchronized(instances) { + instances[cacheDir.absolutePath]?.let { live -> + live.clear() + Result.Success(Unit) + } ?: deleteOnDisk() + } /** - * Returns a [VideoMediaCache] backed by the [SimpleCache] at [cacheDir]. If an instance - * for that absolute directory path already exists in this process, that instance is - * returned and [config] is ignored beyond the first call; this prevents a second - * [SimpleCache] from being constructed against the same directory (which would throw). + * Returns a [VideoMediaCache] backed by the [SimpleCache] at [cacheDir], or `null` if + * [SimpleCache] construction fails (e.g. a stale directory lock left by a prior crash). + * If an instance for that absolute directory path already exists in this process, that + * instance is returned and [config] is ignored beyond the first call; this prevents a + * second [SimpleCache] from being constructed against the same directory (which would throw). * * @param appContext Application context used to construct the [StandaloneDatabaseProvider]. * @param cacheDir Directory that backs the [SimpleCache]. Created if it does not exist. * @param config Cache configuration. Honored only on the first call for [cacheDir]. */ @JvmStatic - public fun create(appContext: Context, cacheDir: File, config: VideoCacheConfig): VideoMediaCache = + public fun create(appContext: Context, cacheDir: File, config: VideoCacheConfig): VideoMediaCache? = synchronized(instances) { cacheDir.mkdirs() val key = cacheDir.absolutePath @@ -146,13 +157,18 @@ public class VideoMediaCache private constructor( } return@synchronized existing } - val dbProvider = StandaloneDatabaseProvider(appContext) - val simpleCache = SimpleCache( - cacheDir, - LeastRecentlyUsedCacheEvictor(config.maxSizeBytes), - dbProvider, - ) - VideoMediaCache(simpleCache, dbProvider, key).also { instances[key] = it } + try { + val dbProvider = StandaloneDatabaseProvider(appContext) + val simpleCache = SimpleCache( + cacheDir, + LeastRecentlyUsedCacheEvictor(config.maxSizeBytes), + dbProvider, + ) + VideoMediaCache(simpleCache, dbProvider, key).also { instances[key] = it } + } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { + logger.e(e) { "[create] Failed to construct SimpleCache at '$key'; video caching disabled." } + null + } } } } diff --git a/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/StreamMediaDataSourceCacheIntegrationTest.kt b/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/StreamMediaDataSourceCacheIntegrationTest.kt index 1055f9a8a5a..b17926081f4 100644 --- a/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/StreamMediaDataSourceCacheIntegrationTest.kt +++ b/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/StreamMediaDataSourceCacheIntegrationTest.kt @@ -29,6 +29,7 @@ import io.getstream.chat.android.client.cache.VideoCacheConfig import io.getstream.chat.android.client.cdn.CDN import io.getstream.chat.android.client.cdn.CDNRequest import io.getstream.chat.android.client.cdn.internal.CDNDataSourceFactory +import io.getstream.result.Result import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -63,7 +64,7 @@ internal class StreamMediaDataSourceCacheIntegrationTest { @Before fun setUp() { cacheDir.deleteRecursively() - cache = VideoMediaCache.create(context, cacheDir, VideoCacheConfig()) + cache = VideoMediaCache.create(context, cacheDir, VideoCacheConfig())!! } @After @@ -187,13 +188,13 @@ internal class StreamMediaDataSourceCacheIntegrationTest { } /** - * `clearAll()` fans out over every live cache in the process registry and empties each in - * place. Here the single registered cache (created in [setUp]) is populated with real bytes; - * after `clearAll()` its content is gone, the call reports it cleared something (`true`), and - * the still-alive `SimpleCache` can serve a fresh write. + * When a live [VideoMediaCache] owns the directory, `clearOrDelete` empties it in place (see + * [VideoMediaCache.clear]) and never runs the on-disk delete. Here the single registered cache + * (created in [setUp]) is populated with real bytes; after `clearOrDelete` its content is gone, + * the delete lambda was not invoked, and the still-alive `SimpleCache` can serve a fresh write. */ @Test - fun `clearAll empties the live cache in place and reports it cleared`() { + fun `clearOrDelete empties the live cache in place and skips the on-disk delete`() { // Arrange: write real bytes into the cache registered in setUp() and confirm they landed. val upstream = RecordingDataSourceFactory() val factory = VideoCacheDataSourceFactory(cache, upstream) @@ -202,35 +203,52 @@ internal class StreamMediaDataSourceCacheIntegrationTest { assertTrue("Precondition: cache tracks the written key", cache.cache.keys.isNotEmpty()) assertTrue("Precondition: cache reports non-zero size", cache.cache.cacheSpace > 0L) - // Act: clear every live cache in the process. - val cleared = VideoMediaCache.clearAll() + // Act: clear the directory owned by the live cache; the on-disk delete must not run. + var deleteInvoked = false + val result = VideoMediaCache.clearOrDelete(cacheDir) { + deleteInvoked = true + Result.Success(Unit) + } - // Assert: it reported clearing a live cache, and this cache is now empty. - assertTrue("clearAll should report that a live cache was cleared", cleared) - assertTrue("Cache should track zero keys after clearAll", cache.cache.keys.isEmpty()) - assertEquals("Cache should report zero size after clearAll", 0L, cache.cache.cacheSpace) + // Assert: cleared in place, delete skipped, and this cache is now empty. + assertTrue("clearOrDelete should succeed", result is Result.Success) + assertFalse("On-disk delete must be skipped when a live cache owns the directory", deleteInvoked) + assertTrue("Cache should track zero keys after clearOrDelete", cache.cache.keys.isEmpty()) + assertEquals("Cache should report zero size after clearOrDelete", 0L, cache.cache.cacheSpace) // Assert: the SimpleCache stays alive, so a subsequent read re-fetches and re-populates. populateCache(factory, VIDEO_URL) - assertEquals("A read after clearAll should hit upstream again", 2, upstream.openCount) + assertEquals("A read after clearOrDelete should hit upstream again", 2, upstream.openCount) assertTrue("Cache should be re-populated after the second read", cache.cache.keys.isNotEmpty()) } /** - * When no live cache is registered in the process (e.g. the app never opted in, or a prior - * cache was released), `clearAll()` has nothing to empty and reports `false` — the signal - * `ChatClient` uses to fall back to deleting the cache directory from disk. + * When no live cache owns the directory (e.g. the app never opted in, or a prior cache was + * released), `clearOrDelete` runs the on-disk delete against that directory instead of clearing + * a live cache in place. */ @Test - fun `clearAll reports nothing to clear when no live cache is registered`() { - // Arrange: release the only registered cache so the process registry is empty. + fun `clearOrDelete deletes on disk when no live cache owns the directory`() { + // Arrange: release the only registered cache so no live cache owns cacheDir, and confirm + // the directory still exists on disk. cache.release() + assertTrue("Precondition: cache directory exists on disk", cacheDir.exists()) + + // Act: clear the now-unowned directory; the delete lambda should run. + var deleteInvoked = false + val result = VideoMediaCache.clearOrDelete(cacheDir) { + deleteInvoked = true + cacheDir.deleteRecursively() + Result.Success(Unit) + } - // Act + Assert. - assertFalse("clearAll should report nothing was cleared on an empty registry", VideoMediaCache.clearAll()) + // Assert: the delete ran and removed the directory from disk. + assertTrue("clearOrDelete should succeed", result is Result.Success) + assertTrue("Delete lambda should run when no live cache owns the directory", deleteInvoked) + assertFalse("Cache directory should be gone after clearOrDelete", cacheDir.exists()) // Recreate so tearDown() releases a live instance and unlocks the directory for the next test. - cache = VideoMediaCache.create(context, cacheDir, VideoCacheConfig()) + cache = VideoMediaCache.create(context, cacheDir, VideoCacheConfig())!! } /** @@ -246,7 +264,7 @@ internal class StreamMediaDataSourceCacheIntegrationTest { context, evictionDir, VideoCacheConfig(maxSizeBytes = 2 * TOTAL_LENGTH), - ) + )!! try { val upstream = RecordingDataSourceFactory() val factory = VideoCacheDataSourceFactory(evictionCache, upstream) @@ -255,7 +273,8 @@ internal class StreamMediaDataSourceCacheIntegrationTest { // Space the operations out so each span's `lastTouchTimestamp` lands in a distinct // millisecond; Media3's LeastRecentlyUsedCacheEvictor uses `System.currentTimeMillis()` // and falls back to alphabetical key order on ties, which would pick A over B under - // load and make the assertions non-deterministic. + // load and make the assertions non-deterministic. The sleeps are wall-clock dependent; + // if this test becomes flaky on a slow runner, increase SPAN_SPACING_MS. readFully(factory.createDataSource(), DataSpec(Uri.parse(VIDEO_A_URL))) Thread.sleep(SPAN_SPACING_MS) readFully(factory.createDataSource(), DataSpec(Uri.parse(VIDEO_B_URL))) diff --git a/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/VideoMediaCacheTest.kt b/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/VideoMediaCacheTest.kt index 9c6d9251cc1..a122add0e03 100644 --- a/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/VideoMediaCacheTest.kt +++ b/stream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/VideoMediaCacheTest.kt @@ -45,7 +45,7 @@ internal class VideoMediaCacheTest { @Before fun setUp() { cacheDir.deleteRecursively() - cache = VideoMediaCache.create(context, cacheDir, VideoCacheConfig()) + cache = VideoMediaCache.create(context, cacheDir, VideoCacheConfig())!! } @After @@ -84,7 +84,7 @@ internal class VideoMediaCacheTest { assertTrue("Expected a different instance after release", recreated !== cache) // Reassign so @After releases the recreated instance and Media3's SimpleCache // unlocks the directory; otherwise the next test's setUp would throw. - cache = recreated + cache = recreated!! } private companion object {