From bc00bc1eb11b32829c1525ade626c5a1c887cef7 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Wed, 29 Jul 2026 14:13:42 -0500 Subject: [PATCH] Stop re-registering the device once per playlist item MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PlaylistController.next() asks for a playlist refresh on every item advance, and requestPlaylistRefresh() emits a full device:register. The server's register handler runs 7+ statements plus the identity/fingerprint path and rebuilds the playlist payload, then pushes the whole playlist back down. So a panel showing a 10-second image re-registered six times a minute, indefinitely, and each reply fed a fresh playlist into a controller that had to diff it — which is what kept the #234 restart loop supplied. It was buying nothing. The heartbeat already refreshes every 4th beat (60s), so the periodic pull this duplicated happens either way. Throttled at the single chokepoint rather than by editing callers, because the callers have genuinely different intents — network-came-back, service-connected, per-item, and the heartbeat itself — and ranking them would be guesswork. A shared floor keeps every caller's meaning: recovery paths still refresh, they just cannot stack. The window sits just under the heartbeat's own 60s so the two interleave instead of the throttle systematically eating the pull we are relying on. Measured on the reproduction over 240s: 9 registrations for 9 item plays before, 3 for the same 9 plays after, with playback unchanged. The saving scales with how short the items are — a 10s item goes from six refreshes a minute to about one. Does NOT change what a refresh does, only how often one may be asked for. --- .../player/service/RefreshThrottle.kt | 28 ++++++++ .../player/service/WebSocketService.kt | 9 +++ .../player/service/RefreshThrottleTest.kt | 64 +++++++++++++++++++ 3 files changed, 101 insertions(+) create mode 100644 android/app/src/main/java/com/remotedisplay/player/service/RefreshThrottle.kt create mode 100644 android/app/src/test/java/com/remotedisplay/player/service/RefreshThrottleTest.kt 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) + } +}