diff --git a/app/src/main/java/com/m57/hermescontrol/notification/ChatNotificationService.kt b/app/src/main/java/com/m57/hermescontrol/notification/ChatNotificationService.kt index 0d3d1503..5bfd9fda 100644 --- a/app/src/main/java/com/m57/hermescontrol/notification/ChatNotificationService.kt +++ b/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( diff --git a/app/src/main/java/com/m57/hermescontrol/notification/ForegroundServiceLifecycle.kt b/app/src/main/java/com/m57/hermescontrol/notification/ForegroundServiceLifecycle.kt new file mode 100644 index 00000000..8409750b --- /dev/null +++ b/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 + } + } +} diff --git a/app/src/test/java/com/m57/hermescontrol/notification/ChatNotificationServiceTest.kt b/app/src/test/java/com/m57/hermescontrol/notification/ChatNotificationServiceTest.kt index 7d406b81..4d4fca1c 100644 --- a/app/src/test/java/com/m57/hermescontrol/notification/ChatNotificationServiceTest.kt +++ b/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(relaxed = true) + + NotificationHelper.stop(context) + + verify(exactly = 0) { context.stopService(any()) } + } + @Test fun `setAppForeground true sets the flag`() { val field = diff --git a/app/src/test/java/com/m57/hermescontrol/notification/ForegroundServiceLifecycleTest.kt b/app/src/test/java/com/m57/hermescontrol/notification/ForegroundServiceLifecycleTest.kt new file mode 100644 index 00000000..70d01548 --- /dev/null +++ b/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() + 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() + 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) + } +}