Merge branch 'dev' into fix/rotation-preserve-connection

app/src/main/java/com/m57/hermescontrol/notification/ChatNotificationService.kt

@@ -66,6 +66,7 @@ class ChatNotificationService : Service() {
         internal const val PENDING_NOTIFICATION_ID = 2
 
         private val isAppInForeground = AtomicBoolean(false)
+        internal val lifecycle = ForegroundServiceLifecycle()
 
         fun setAppForeground(foreground: Boolean) {
             isAppInForeground.set(foreground)
@@ -80,7 +81,6 @@ class ChatNotificationService : Service() {
     override fun onCreate() {
         super.onCreate()
         createNotificationChannels()
-        startForeground(NOTIFICATION_ID, buildForegroundNotification(getString(R.string.notif_waiting_replies)))
         startEventCollection()
     }
 
@@ -89,6 +89,7 @@ class ChatNotificationService : Service() {
             serviceScope.launch {
                 HermesWsClient.events.collect { event ->
                     if (!isAppInForeground.get()) {
+                        val generation = lifecycle.generation
                         launch {
                             delay(500)
                             if (!isAppInForeground.get()) {
@@ -111,8 +112,8 @@ class ChatNotificationService : Service() {
                                         // HermesWsClient's own collector, so the
                                         // service is not restarted on the next
                                         // ON_STOP (issue #794).
-                                        stopForeground(STOP_FOREGROUND_REMOVE)
-                                        stopSelf()
+                                        // A delayed completion must not retire a newer turn/start.
+                                        if (!HermesWsClient.pendingReply) lifecycle.complete(generation)
                                     }
 
                                     is WsEvent.ClarifyRequest -> {
@@ -260,11 +261,23 @@ class ChatNotificationService : Service() {
         intent: Intent?,
         flags: Int,
         startId: Int,
-    ): Int = START_NOT_STICKY
+    ): Int {
+        lifecycle.onStart(
+            owner = this,
+            promote = {
+                startForeground(NOTIFICATION_ID, buildForegroundNotification(getString(R.string.notif_waiting_replies)))
+            },
+            // Do not remove foreground status before Android accepts retirement:
+            // a newer start may already be queued, but not delivered to us yet.
+            stopLatest = { stopSelfResult(startId) },
+        )
+        return START_NOT_STICKY
+    }
 
     override fun onBind(intent: Intent?): IBinder? = null
 
     override fun onDestroy() {
+        lifecycle.onDestroyed(this)
         eventCollector?.cancel()
         serviceScope.cancel()
         super.onDestroy()
@@ -283,15 +296,19 @@ object NotificationHelper {
         // when nothing is in flight.
         if (!HermesWsClient.pendingReply) return
         val intent = Intent(context, ChatNotificationService::class.java)
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
-            context.startForegroundService(intent)
-        } else {
-            context.startService(intent)
+        ChatNotificationService.lifecycle.start {
+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+                context.startForegroundService(intent)
+            } else {
+                context.startService(intent)
+            }
         }
     }
 
     fun stop(context: Context) {
-        context.stopService(Intent(context, ChatNotificationService::class.java))
+        // Rotation can resume the activity before the queued service is created.
+        // Direct stopService here crashes Android while foreground promotion is owed.
+        ChatNotificationService.lifecycle.stop()
     }
 
     fun setAppForeground(

app/src/main/java/com/m57/hermescontrol/notification/ForegroundServiceLifecycle.kt

@@ -0,0 +1,72 @@
+package com.m57.hermescontrol.notification
+
+/**
+ * Orders foreground-service requests and retirement (rotation/start-stop regression).
+ * A stop before creation is deferred until promotion. Android's stopSelfResult(startId)
+ * protects a delivered start from tearing down a newer start still queued by the system.
+ */
+internal class ForegroundServiceLifecycle {
+    private var requested = false
+    private var owner: Any? = null
+    private var stopCurrent: (() -> Boolean)? = null
+
+    var generation = 0L
+        @Synchronized get
+        private set
+
+    @Synchronized
+    fun start(request: () -> Unit) {
+        val wasRequested = requested
+        val previousGeneration = generation
+        generation++
+        requested = true
+        try {
+            request()
+        } catch (error: Throwable) {
+            requested = wasRequested
+            generation = previousGeneration
+            throw error
+        }
+    }
+
+    @Synchronized
+    fun stop() {
+        requested = false
+        retire()
+    }
+
+    @Synchronized
+    fun onStart(
+        owner: Any,
+        promote: () -> Unit,
+        stopLatest: () -> Boolean,
+    ) {
+        // Fulfil each startForegroundService request before allowing any retirement.
+        promote()
+        this.owner = owner
+        stopCurrent = stopLatest
+        if (!requested) retire()
+    }
+
+    @Synchronized
+    fun complete(expectedGeneration: Long) {
+        if (generation != expectedGeneration) return
+        requested = false
+        retire()
+    }
+
+    @Synchronized
+    fun onDestroyed(owner: Any) {
+        if (this.owner === owner) {
+            this.owner = null
+            stopCurrent = null
+        }
+    }
+
+    private fun retire() {
+        if (stopCurrent?.invoke() == true) {
+            owner = null
+            stopCurrent = null
+        }
+    }
+}

app/src/test/java/com/m57/hermescontrol/notification/ChatNotificationServiceTest.kt

@@ -1,5 +1,8 @@
 package com.m57.hermescontrol.notification
 
+import android.content.Context
+import io.mockk.mockk
+import io.mockk.verify
 import org.junit.Assert.assertEquals
 import org.junit.Test
 
@@ -17,6 +20,15 @@ import org.junit.Test
  * or through the public `setAppForeground` API.
  */
 class ChatNotificationServiceTest {
+    @Test
+    fun `stop never tears down a service awaiting foreground promotion`() {
+        val context = mockk<Context>(relaxed = true)
+
+        NotificationHelper.stop(context)
+
+        verify(exactly = 0) { context.stopService(any()) }
+    }
+
     @Test
     fun `setAppForeground true sets the flag`() {
         val field =

app/src/test/java/com/m57/hermescontrol/notification/ForegroundServiceLifecycleTest.kt

@@ -0,0 +1,113 @@
+package com.m57.hermescontrol.notification
+
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+class ForegroundServiceLifecycleTest {
+    @Test
+    fun `stop before creation waits for promotion`() {
+        val lifecycle = ForegroundServiceLifecycle()
+        val calls = mutableListOf<String>()
+        lifecycle.start { calls += "request" }
+        lifecycle.stop()
+        assertEquals(listOf("request"), calls)
+        lifecycle.onStart(Any(), { calls += "promote" }) {
+            calls += "stop"
+            true
+        }
+        assertEquals(listOf("request", "promote", "stop"), calls)
+    }
+
+    @Test
+    fun `every delivered start promotes an existing instance`() {
+        val lifecycle = ForegroundServiceLifecycle()
+        val owner = Any()
+        var promotions = 0
+        lifecycle.start {}
+        repeat(2) { lifecycle.onStart(owner, { promotions++ }) { error("Unexpected stop") } }
+        assertEquals(2, promotions)
+    }
+
+    @Test
+    fun `a queued newer start cannot be stopped with an older start id`() {
+        val lifecycle = ForegroundServiceLifecycle()
+        val calls = mutableListOf<String>()
+        val owner = Any()
+        lifecycle.start {}
+        lifecycle.onStart(owner, { calls += "promote1" }) {
+            calls += "stop1-rejected"
+            false // Android stopSelfResult rejects an ID older than its latest queued start.
+        }
+        lifecycle.start {}
+        lifecycle.stop()
+        lifecycle.onStart(owner, { calls += "promote2" }) {
+            calls += "stop2"
+            true
+        }
+        assertEquals(listOf("promote1", "stop1-rejected", "promote2", "stop2"), calls)
+    }
+
+    @Test
+    fun `delayed completion cannot retire a newer request`() {
+        val lifecycle = ForegroundServiceLifecycle()
+        var stops = 0
+        lifecycle.start {}
+        lifecycle.onStart(Any(), {}) {
+            stops++
+            true
+        }
+        val oldGeneration = lifecycle.generation
+        lifecycle.start {}
+        lifecycle.complete(oldGeneration)
+        assertEquals(0, stops)
+        lifecycle.complete(lifecycle.generation)
+        assertEquals(1, stops)
+    }
+
+    @Test
+    fun `stop followed by a new start before creation keeps service alive`() {
+        val lifecycle = ForegroundServiceLifecycle()
+        lifecycle.start {}
+        lifecycle.stop()
+        lifecycle.start {}
+        lifecycle.onStart(Any(), {}) { error("New request must survive") }
+    }
+
+    @Test
+    fun `destroying an old instance does not detach its replacement`() {
+        val lifecycle = ForegroundServiceLifecycle()
+        val old = Any()
+        var stops = 0
+        lifecycle.start {}
+        lifecycle.onStart(old, {}) { true }
+        lifecycle.onStart(Any(), {}) {
+            stops++
+            true
+        }
+        lifecycle.onDestroyed(old)
+        lifecycle.stop()
+        assertEquals(1, stops)
+    }
+
+    @Test
+    fun `idle stop does not launch a service`() {
+        ForegroundServiceLifecycle().stop()
+    }
+
+    @Test
+    fun `rejected start preserves completion of the existing request`() {
+        val lifecycle = ForegroundServiceLifecycle()
+        var stops = 0
+        lifecycle.start {}
+        lifecycle.onStart(Any(), {}) {
+            stops++
+            true
+        }
+        val generation = lifecycle.generation
+        val failure = IllegalStateException("Start rejected")
+        val result = runCatching { lifecycle.start { throw failure } }
+        assertEquals(failure, result.exceptionOrNull())
+        lifecycle.complete(generation)
+        assertEquals(1, stops)
+    }
+}