diff --git a/android/app/src/main/java/com/remotedisplay/player/service/RefreshThrottle.kt b/android/app/src/main/java/com/remotedisplay/player/service/RefreshThrottle.kt new file mode 100644 index 0000000..dcd4903 --- /dev/null +++ b/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 + } +} diff --git a/android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt b/android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt index 225f675..a350f2b 100644 --- a/android/app/src/main/java/com/remotedisplay/player/service/WebSocketService.kt +++ b/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 { diff --git a/android/app/src/test/java/com/remotedisplay/player/service/RefreshThrottleTest.kt b/android/app/src/test/java/com/remotedisplay/player/service/RefreshThrottleTest.kt new file mode 100644 index 0000000..9c6834d --- /dev/null +++ b/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) + } +}