diff --git a/android/app/src/main/java/com/remotedisplay/player/service/ConnectionGuard.kt b/android/app/src/main/java/com/remotedisplay/player/service/ConnectionGuard.kt new file mode 100644 index 0000000..78a3cc0 --- /dev/null +++ b/android/app/src/main/java/com/remotedisplay/player/service/ConnectionGuard.kt @@ -0,0 +1,29 @@ +package com.remotedisplay.player.service + +/** + * #148 root-cause guard: the SINGLE-SOCKET-PER-DEVICE invariant, extracted from + * [WebSocketService] so it is unit-testable without a live Socket.IO / Android (the service is + * just the shell — same pattern as OtaThrottle). It decides whether a connect() request should + * OPEN a new socket or REUSE the one already held. + * + * WHY (#148): every entry point — BOOT_COMPLETED -> service start, Activity bind + * (onServiceConnected), a foreground re-bind (the MAXHUB PROC_STATE_TOP isBindService:true + * transition), Socket.IO reconnect — used to call an UNCONDITIONAL connect() that tore down any + * healthy socket and opened a forceNew one. A ROM that re-binds on foreground therefore produced + * a burst of sockets for the same device_id (the 8-in-9s storm). Fire TV never re-binds like + * that, so it never reproduced. This guards the SOCKET (the thing that matters), not the + * service/bind count (the thing that varies), so it closes every duplication vector at once. + */ +object ConnectionGuard { + /** + * Should connect(url) open a NEW socket, or reuse the current one? + * + * Reuse (return false) iff we already hold a socket to the SAME url that is live or + * self-healing — [socketActive] means connected OR Socket.IO is auto-reconnecting it. Open a + * new one (return true) only when there is none usable: no socket, a different url (a genuine + * re-provision to another server), or the socket went inert (e.g. after `io server + * disconnect`, which Socket.IO does not auto-reconnect). + */ + fun shouldOpenNewSocket(hasSocket: Boolean, sameUrl: Boolean, socketActive: Boolean): Boolean = + !(hasSocket && sameUrl && socketActive) +} 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 68b6e23..950a2f8 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 @@ -22,12 +22,25 @@ import java.net.URI class WebSocketService : Service() { private var socket: Socket? = null + // #148 root-cause guard: the single-socket invariant. currentUrl + socketActive track the + // ONE socket so connect() is idempotent across every entry point (boot, service start, + // activity bind, foreground re-bind) — a re-bind can never open a duplicate. See + // ConnectionGuard. socketActive == "connected OR Socket.IO is auto-reconnecting it". + @Volatile private var socketActive = false + @Volatile private var currentUrl: String? = null + private var reopenRunnable: Runnable? = null private lateinit var config: ServerConfig private lateinit var deviceInfo: DeviceInfo private val handler = Handler(Looper.getMainLooper()) private var heartbeatRunnable: Runnable? = null private val binder = LocalBinder() + companion object { + // #148: backoff before re-opening the single socket after a disconnect that Socket.IO + // does NOT auto-reconnect (io server/client disconnect) — never a blind immediate re-open. + private const val RECONNECT_AFTER_EVICT_MS = 3000L + } + // Callbacks var onPaired: ((String, String) -> Unit)? = null var onUnpaired: (() -> Unit)? = null @@ -76,6 +89,10 @@ class WebSocketService : Service() { } override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { + // #148 single owner: the SERVICE owns the one connection. Idempotent (ConnectionGuard), + // so a START_STICKY restart / re-delivery / boot start reuses a live socket and never + // opens a duplicate. No-op until a server url is configured (provisioning). + connect() return START_STICKY } @@ -94,14 +111,31 @@ class WebSocketService : Service() { return this } + /** + * Idempotent connect — the #148 root-cause guard. Safe to call from EVERY entry point + * (service start, activity bind, foreground transition, reconnect). If we already hold a + * live or self-healing socket to the same url, REUSE it; only ever open ONE socket per + * device. @Synchronized so racing entry points can't open two. + */ + @Synchronized fun connect(serverUrl: String? = null) { val url = serverUrl ?: config.serverUrl if (url.isEmpty()) { Log.e("WebSocketService", "No server URL configured") return } + if (!ConnectionGuard.shouldOpenNewSocket(socket != null, currentUrl == url, socketActive)) { + Log.i("WebSocketService", "connect(): reusing existing socket to $url — no duplicate (#148)") + return + } + openSocket(url) + } + @Synchronized + private fun openSocket(url: String) { disconnect() + currentUrl = url + socketActive = true try { val options = IO.Options().apply { @@ -126,8 +160,16 @@ class WebSocketService : Service() { val reason = args.firstOrNull()?.toString() ?: "unknown" Log.w("WebSocketService", "Disconnected from server: $reason") // Stop heartbeat while disconnected; player keeps showing cached content. - // Socket.IO will reconnect automatically per the options above. stopHeartbeat() + // #148 reconnect discipline: Socket.IO auto-reconnects the SAME socket on a + // transport drop (reconnection=true) — leave socketActive true so connect() + // keeps reusing it. But on a server- or client-initiated disconnect it does + // NOT auto-reconnect, so mark the socket inert and bring up exactly ONE new + // connection after a backoff — never a blind re-open that gets evicted again. + if (reason == "io server disconnect" || reason == "io client disconnect") { + socketActive = false + scheduleReopen(RECONNECT_AFTER_EVICT_MS) + } } safeOn(Socket.EVENT_CONNECT_ERROR) { args -> @@ -613,11 +655,28 @@ class WebSocketService : Service() { fun disconnect() { stopHeartbeat() + cancelReopen() + socketActive = false try { socket?.disconnect() } catch (e: Throwable) { Log.w("WebSocketService", "disconnect: ${e.message}") } try { socket?.off() } catch (e: Throwable) { Log.w("WebSocketService", "off: ${e.message}") } socket = null } + // #148 reconnect discipline: bring up exactly ONE new connection after an eviction, with a + // backoff, and only one pending at a time — so a server eviction can't trigger a blind + // re-open loop. connect() itself is idempotent, so this is safe even if an activity rebind + // races it. + private fun scheduleReopen(delayMs: Long) { + if (reopenRunnable != null) return + val r = Runnable { reopenRunnable = null; connect() } + reopenRunnable = r + handler.postDelayed(r, delayMs) + } + private fun cancelReopen() { + reopenRunnable?.let { handler.removeCallbacks(it) } + reopenRunnable = null + } + fun isConnected(): Boolean = socket?.connected() == true override fun onDestroy() { diff --git a/android/app/src/test/java/com/remotedisplay/player/service/ConnectionGuardTest.kt b/android/app/src/test/java/com/remotedisplay/player/service/ConnectionGuardTest.kt new file mode 100644 index 0000000..b14f248 --- /dev/null +++ b/android/app/src/test/java/com/remotedisplay/player/service/ConnectionGuardTest.kt @@ -0,0 +1,47 @@ +package com.remotedisplay.player.service + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * #148: the single-socket-per-device invariant (root-cause fix for the client opening + * duplicate/rapid sockets). ConnectionGuard is the testable core; WebSocketService is the shell. + */ +class ConnectionGuardTest { + + @Test fun opensWhenThereIsNoSocket() { + assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = false, sameUrl = false, socketActive = false)) + assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = false, sameUrl = true, socketActive = false)) + } + + @Test fun reusesALiveSocketToTheSameUrl() { + // A connected/self-healing socket to the same server -> reuse, never a duplicate. + assertFalse(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = true, socketActive = true)) + } + + @Test fun opensWhenTheSocketWentInert() { + // Socket exists but is inert (e.g. after `io server disconnect`, which Socket.IO does + // not auto-reconnect) -> bring up one new connection. + assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = true, socketActive = false)) + } + + @Test fun opensWhenTheUrlChanged() { + // Genuine re-provision to a different server -> switch (do not reuse the old one). + assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = false, socketActive = true)) + assertTrue(ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = false, socketActive = false)) + } + + /** + * THE #148 case: repeated connect() from repeated Activity binds / foreground re-binds must + * never open a second socket while one is active. Simulate the 8-in-9s storm of binds. + */ + @Test fun idempotentAcrossManyRapidBinds() { + repeat(8) { i -> + assertFalse( + "bind #$i must reuse the live socket, not open a duplicate", + ConnectionGuard.shouldOpenNewSocket(hasSocket = true, sameUrl = true, socketActive = true) + ) + } + } +}