diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/playerPool/SessionRegistry.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/playerPool/SessionRegistry.kt new file mode 100644 index 0000000000..768a420458 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/playerPool/SessionRegistry.kt @@ -0,0 +1,140 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.service.playback.playerPool + +import androidx.collection.LruCache + +/** + * Owns which sessions are reachable, and is the only place that decides one has been dropped. + * + * Two tiers: an LRU of idle entries and a map of ids that are currently playing. A playing entry + * stays in [idle] as well — [playing] only makes an eviction non-dropping. That is deliberate: it + * matches today's behaviour exactly (a playing session keeps consuming a cache slot until it ages + * out), and it is what keeps the replacement guard in [entryRemoved] load-bearing rather than dead + * code. + * + * Generic over the entry type so the bookkeeping — the part that has actually been wrong — is unit + * testable on the JVM with no Android or media3 objects involved. + * + * Uses `androidx.collection.LruCache` rather than `android.util.LruCache`: the latter is stubbed + * out under `unitTests.isReturnDefaultValues = true`, which would make every test here vacuous. + * The contract is otherwise identical, including `entryRemoved` firing outside the lock. + * + * Not thread safe. Every caller runs on the main thread (see MediaSessionPool). + */ +internal class SessionRegistry( + maxIdle: Int, + private val onDropped: (T) -> Unit, +) { + private val playing = mutableMapOf() + + // Set while an explicit drop is tearing an entry out of [idle], so the resulting entryRemoved + // callback doesn't signal a second time for the same drop. + // + // Resetting to false in `finally` (rather than restoring a saved value) is sound because the + // guarded region cannot nest: onDropped is never invoked while the flag is set, so nothing + // inside it can re-enter drop()/dropAll(). That non-nesting property is the invariant — not + // single-threadedness — and a future edit that called onDropped inside the guarded region + // would break it. + private var suppressDropSignal = false + + private val idle = + object : LruCache(maxIdle) { + override fun entryRemoved( + evicted: Boolean, + key: String, + oldValue: T, + newValue: T?, + ) { + // A replacement is not a drop: re-putting the same entry (pause -> idle) fires this + // with newValue === oldValue, and retiring then would kill the session we are + // keeping. `newValue == null` needs no separate test — V is non-null by type, so a + // null newValue can never be identical to oldValue. + if (suppressDropSignal) return + if (newValue !== oldValue && !playing.containsKey(key)) onDropped(oldValue) + } + } + + fun register( + id: String, + entry: T, + ) { + idle.put(id, entry) + } + + fun setPlaying( + id: String, + isPlaying: Boolean, + ) { + val entry = get(id) ?: return + if (isPlaying) { + playing[id] = entry + } else { + // Re-inserts if it was evicted from idle while playing. Must happen before the playing + // entry is cleared, so the eviction this put may trigger still sees the pin. + idle.put(id, entry) + playing.remove(id) + } + } + + fun get(id: String): T? = playing[id] ?: idle.get(id) + + fun idleSnapshot(): List = idle.snapshot().values.toList() + + // Copies, like idleSnapshot(). Handing out playing.values would be a live view, and a caller + // that iterated it while a drop fired would get a ConcurrentModificationException. A type whose + // job is owning reachability should not ship that footgun. + fun playingEntries(): List = playing.values.toList() + + /** + * Explicit release. Drops whether or not the session is playing, and does not rely on + * [idle]'s removal firing the callback — that reliance is the leak, because it silently does + * nothing when the entry was already evicted while playing. + */ + fun drop(id: String): T? { + val entry = playing.remove(id) ?: idle.get(id) ?: return null + suppressDropSignal = true + try { + idle.remove(id) + } finally { + suppressDropSignal = false + } + onDropped(entry) + return entry + } + + /** Teardown sweep: every reachable entry is dropped exactly once, playing or idle. */ + fun dropAll() { + val all = ArrayList() + playing.values.forEach { entry -> if (all.none { it === entry }) all.add(entry) } + idle.snapshot().values.forEach { entry -> if (all.none { it === entry }) all.add(entry) } + + playing.clear() + suppressDropSignal = true + try { + idle.evictAll() + } finally { + suppressDropSignal = false + } + + all.forEach(onDropped) + } +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/playback/playerPool/SessionRegistryTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/playback/playerPool/SessionRegistryTest.kt new file mode 100644 index 0000000000..3fea991a07 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/playback/playerPool/SessionRegistryTest.kt @@ -0,0 +1,165 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.service.playback.playerPool + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotSame +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Test + +class SessionRegistryTest { + private val dropped = mutableListOf() + + private fun registry(maxIdle: Int) = SessionRegistry(maxIdle) { dropped.add(it) } + + @Test + fun evictionDropsTheOldestExactlyOnce() { + val registry = registry(maxIdle = 1) + registry.register("a", "A") + registry.register("b", "B") + assertEquals(listOf("A"), dropped) + } + + @Test + fun playingSessionSurvivesEviction() { + val registry = registry(maxIdle = 1) + registry.register("a", "A") + registry.setPlaying("a", true) + registry.register("b", "B") + assertEquals(emptyList(), dropped) + } + + // The replacement guard. setPlaying(false) re-puts the SAME entry, which fires + // entryRemoved(newValue === oldValue). Treating that as a drop would retire the + // session we are trying to keep. + @Test + fun pauseDoesNotDropTheSessionItIsKeeping() { + val registry = registry(maxIdle = 2) + registry.register("a", "A") + registry.setPlaying("a", true) + registry.setPlaying("a", false) + assertEquals(emptyList(), dropped) + } + + @Test + fun pausedSessionBecomesEvictableAgain() { + val registry = registry(maxIdle = 1) + registry.register("a", "A") + registry.setPlaying("a", true) + registry.register("b", "B") // A survives, but leaves idle + registry.setPlaying("a", false) // A re-enters idle and displaces B + assertEquals(listOf("B"), dropped) + } + + // Leak path 2: an explicit release must drop even while playing. + @Test + fun explicitDropOfPlayingSessionDropsExactlyOnce() { + val registry = registry(maxIdle = 2) + registry.register("a", "A") + registry.setPlaying("a", true) + registry.drop("a") + assertEquals(listOf("A"), dropped) + } + + // Leak path 2, the exact case the old cache.remove(id) missed: already evicted + // from idle while playing, so there is no cache entry left to trigger the callback. + @Test + fun explicitDropAfterEvictionWhilePlayingStillDrops() { + val registry = registry(maxIdle = 1) + registry.register("a", "A") + registry.setPlaying("a", true) + registry.register("b", "B") + registry.drop("a") + assertEquals(listOf("A"), dropped) + } + + @Test + fun dropAllDropsEveryEntryExactlyOnce() { + val registry = registry(maxIdle = 3) + registry.register("a", "A") + registry.register("b", "B") + registry.register("c", "C") + registry.setPlaying("b", true) + registry.dropAll() + assertEquals(3, dropped.size) + assertEquals(setOf("A", "B", "C"), dropped.toSet()) + } + + @Test + fun dropAllIncludesSessionEvictedWhilePlaying() { + val registry = registry(maxIdle = 1) + registry.register("a", "A") + registry.setPlaying("a", true) + registry.register("b", "B") + registry.dropAll() + assertEquals(2, dropped.size) + assertEquals(setOf("A", "B"), dropped.toSet()) + } + + @Test + fun getFindsPlayingAndIdleEntries() { + val registry = registry(maxIdle = 2) + registry.register("a", "A") + registry.register("b", "B") + registry.setPlaying("a", true) + assertSame("A", registry.get("a")) + assertSame("B", registry.get("b")) + assertNull(registry.get("missing")) + } + + @Test + fun droppingUnknownIdIsANoOp() { + val registry = registry(maxIdle = 2) + assertNull(registry.drop("missing")) + assertEquals(emptyList(), dropped) + } + + @Test + fun enumerationSeesIdleAndPlayingSeparately() { + val registry = registry(maxIdle = 2) + registry.register("a", "A") + registry.register("b", "B") + registry.setPlaying("a", true) + assertEquals(setOf("A", "B"), registry.idleSnapshot().toSet()) + assertEquals(listOf("A"), registry.playingEntries().toList()) + } + + // The replacement guard uses !== (identity, not equality). This test pins that distinction: + // replacing an entry with an equal-but-not-identical instance must drop the old one. + // Changing !== to != would break this test, even though all existing tests would pass. + @Test + fun replacementWithEqualButDifferentInstanceDropsOldEntry() { + val registry = registry(maxIdle = 2) + val original = "A" + registry.register("a", original) + + // Create a new String instance that is equal to but not identical to the original. + // String literals are interned, so we must construct it explicitly. + val replacement = buildString { append("A") } + assertEquals("Equal values", original, replacement) + assertNotSame("Not the same instance", original, replacement) + + // Registering the replacement should drop the original. + registry.register("a", replacement) + assertEquals(listOf(original), dropped) + } +}