Merge branch 'fix/playlist-refresh-not-once-per-item'

android/app/src/main/java/com/remotedisplay/player/service/RefreshThrottle.kt

@@ -0,0 +1,28 @@
+package com.remotedisplay.player.service
+
+/**
+ * #234 follow-up: how often a device may ask the server to re-send its playlist.
+ *
+ * "Refresh" is not cheap. requestPlaylistRefresh() emits a full `device:register`, and the server's
+ * register handler runs 7+ statements plus the whole fingerprint/identity path and rebuilds the
+ * playlist payload. PlaylistController.next() was calling it on EVERY item advance, so a panel on a
+ * 10-second image re-registered six times a minute, forever — and each reply pushed a full playlist
+ * back down, which is what kept feeding the restart loop behind #234.
+ *
+ * It was also redundant: the heartbeat already refreshes every 4th beat (60s), so the periodic pull
+ * this was duplicating exists either way. Throttling at the single chokepoint keeps every caller's
+ * intent — recovery paths still refresh, they just cannot stack up — without having to rank them.
+ *
+ * Pure so the interval arithmetic is testable without a device or a socket.
+ */
+object RefreshThrottle {
+    /** Just under the heartbeat's own 60s pull, so the two interleave instead of cancelling out. */
+    const val MIN_INTERVAL_MS = 55_000L
+
+    fun shouldRefresh(lastAtMs: Long, nowMs: Long): Boolean {
+        if (lastAtMs <= 0L) return true          // never refreshed — always allow the first
+        val since = nowMs - lastAtMs
+        if (since < 0L) return true              // clock corrected backwards; never wedge on it
+        return since >= MIN_INTERVAL_MS
+    }
+}

android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt

@@ -845,8 +845,17 @@ class WebSocketService : Service() {
         connect()
     }
 
+    @Volatile private var lastRefreshAt = 0L
+
     fun requestPlaylistRefresh() {
         if (socket?.connected() != true || config.deviceId.isEmpty()) return
+        // #234 follow-up: this emits a FULL device:register (7+ server statements + the identity
+        // path + a playlist rebuild), and PlaylistController.next() calls it on every item advance.
+        // A 10-second image therefore re-registered six times a minute. The heartbeat already pulls
+        // a fresh playlist every 60s, so the per-item call bought nothing and cost a great deal.
+        val now = System.currentTimeMillis()
+        if (!RefreshThrottle.shouldRefresh(lastRefreshAt, now)) return
+        lastRefreshAt = now
         Log.i("WebSocketService", "Requesting playlist refresh")
         try {
             val data = org.json.JSONObject().apply {

android/app/src/test/java/com/remotedisplay/player/service/RefreshThrottleTest.kt

@@ -0,0 +1,64 @@
+package com.remotedisplay.player.service
+
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * #234 follow-up: a playlist refresh emits a full device:register, and PlaylistController.next()
+ * asked for one on EVERY item advance. On a 10-second image that is six full re-registrations a
+ * minute, per device, forever — each running 7+ server statements and the identity path, and each
+ * replying with a full playlist push. Measured on the reproduction: 9 item plays, 9 registrations.
+ *
+ * The heartbeat already pulls a fresh playlist every 60s, so the per-item call was duplicating a
+ * refresh that happens anyway.
+ */
+class RefreshThrottleTest {
+
+    private val NOW = 5_000_000L
+    private val MIN = RefreshThrottle.MIN_INTERVAL_MS
+
+    @Test fun the_very_first_refresh_always_goes_through() {
+        // A device that has never asked must not be held back by an empty timestamp.
+        assertTrue(RefreshThrottle.shouldRefresh(lastAtMs = 0L, nowMs = NOW))
+    }
+
+    @Test fun THE_BUG_a_second_refresh_moments_later_is_suppressed() {
+        // Two item advances a few seconds apart: the second must not re-register.
+        assertFalse(RefreshThrottle.shouldRefresh(NOW - 3_000, NOW))
+        assertFalse(RefreshThrottle.shouldRefresh(NOW - 10_000, NOW))
+    }
+
+    @Test fun once_the_interval_has_passed_it_refreshes_again() {
+        assertTrue(RefreshThrottle.shouldRefresh(NOW - MIN, NOW))
+        assertTrue(RefreshThrottle.shouldRefresh(NOW - (MIN + 1), NOW))
+    }
+
+    @Test fun just_under_the_interval_is_still_suppressed() {
+        assertFalse(RefreshThrottle.shouldRefresh(NOW - (MIN - 1), NOW))
+    }
+
+    @Test fun it_sits_under_the_heartbeat_pull_so_the_two_interleave() {
+        // The heartbeat refreshes every 60s. A window at or above that would systematically
+        // suppress the heartbeat's own pull, which is the one we are relying on to remain.
+        assertTrue(MIN < 60_000L)
+    }
+
+    @Test fun a_backwards_clock_never_wedges_refreshing() {
+        // Signage panels correct their clocks. A future 'last' must not disable refresh until the
+        // clock catches up — that would strand a device on a stale playlist for hours.
+        assertTrue(RefreshThrottle.shouldRefresh(NOW + 3_600_000, NOW))
+    }
+
+    @Test fun a_ten_second_item_collapses_from_six_refreshes_a_minute_to_about_one() {
+        // Walk a minute of 10s items and count what actually gets through.
+        var last = 0L
+        var allowed = 0
+        var t = NOW
+        repeat(6) {
+            if (RefreshThrottle.shouldRefresh(last, t)) { allowed++; last = t }
+            t += 10_000
+        }
+        assertTrue("expected roughly one refresh per minute, got $allowed", allowed <= 2)
+    }
+}