Fix scenes expiration

Author: MilosKozak

core/interfaces/src/main/kotlin/app/aaps/core/interfaces/scenes/SceneAutomationApi.kt

@@ -40,11 +40,29 @@ interface SceneAutomationApi {
     /** Set the enabled flag on a scene by id. Fails only if the scene is missing. */
     fun setEnabled(id: String, enabled: Boolean): SceneAutomationResult
 
-    /** Whether a scene is currently active (running, not yet expired). */
+    /**
+     * Whether a scene is currently active (running, not yet expired).
+     *
+     * Asks "is a scene in force right now", so an expired scene whose effects are already reverted
+     * does NOT count, even while its "ended" banner is still on screen. This is the question the
+     * automation gates ask (`TriggerSceneActive`, and through it `ActionRunScene`'s precondition);
+     * counting the banner there would block them until the user happens to dismiss it.
+     *
+     * For "is there a scene the user can act on", use [hasSceneToStop] instead.
+     */
     fun isAnySceneActive(): Boolean
 
+    /**
+     * Whether [stopActiveScene] has something to act on: a scene that is running, or one that has
+     * expired but whose banner has not been dismissed yet (dismissing it is a valid stop).
+     *
+     * Deliberately wider than [isAnySceneActive], and the same condition [activeFlow] emits — the
+     * wear tile is driven by both, so they must agree.
+     */
+    fun hasSceneToStop(): Boolean
+
     /** Emits true when there's a stoppable scene state — running OR expired-with-banner.
-     *  Used by wear-sync to drive the tile's stop button visibility. */
+     *  Used by wear-sync to drive the tile's stop button visibility. Matches [hasSceneToStop]. */
     val activeFlow: Flow<Boolean>
 
     /** End the active scene (deactivate) or dismiss the expired banner. No-op if nothing active. */

implementation/src/main/kotlin/app/aaps/implementation/scenes/ActiveSceneManager.kt

@@ -69,7 +69,6 @@ class ActiveSceneManager @Inject constructor(
 
     /** Set the active scene state (called by SceneExecutor on activation) */
     fun setActive(state: ActiveSceneState) {
-        aapsLogger.info(LTag.UI, "XXXX ActiveSceneManager.setActive('${state.scene.name}')")
         _activeSceneState.value = state
         persistActiveState(state)
     }
@@ -79,15 +78,13 @@ class ActiveSceneManager @Inject constructor(
     fun markExpired() {
         val current = _activeSceneState.value ?: return
         if (current.lifecycle == SceneLifecycle.EXPIRED) return
-        aapsLogger.info(LTag.UI, "XXXX ActiveSceneManager.markExpired() — scene='${current.scene.name}'")
         val updated = current.copy(lifecycle = SceneLifecycle.EXPIRED)
         _activeSceneState.value = updated
         persistActiveState(updated)
     }
 
     /** Clear the active scene (called by SceneExecutor on deactivation or dismiss) */
     fun clearActive() {
-        aapsLogger.info(LTag.UI, "XXXX ActiveSceneManager.clearActive() — was='${_activeSceneState.value?.scene?.name}'")
         _activeSceneState.value = null
         preferences.put(StringNonKey.ActiveScene, "")
     }

implementation/src/main/kotlin/app/aaps/implementation/scenes/SceneAutomationApiImpl.kt

@@ -28,7 +28,11 @@ class SceneAutomationApiImpl @Inject constructor(
 
     override val scenesFlow: StateFlow<String> get() = sceneRepository.scenesFlow
 
-    override fun isAnySceneActive(): Boolean = activeSceneManager.isActive()
+    // An expired scene keeps its slot until the user dismisses the banner, so "a scene exists" and
+    // "a scene is in force" are different questions. isActive() answers the first one.
+    override fun isAnySceneActive(): Boolean = activeSceneManager.isActive() && !activeSceneManager.isExpired()
+
+    override fun hasSceneToStop(): Boolean = activeSceneManager.isActive()
 
     override val activeFlow: Flow<Boolean> =
         activeSceneManager.activeSceneState.map { it != null }.distinctUntilChanged()

implementation/src/main/kotlin/app/aaps/implementation/scenes/SceneExecutor.kt

@@ -161,7 +161,6 @@ class SceneExecutor @Inject constructor(
      * @return Result of the execution
      */
     suspend fun activate(scene: Scene, durationMinutes: Int = scene.defaultDurationMinutes): SceneExecutionResult {
-        aapsLogger.info(LTag.UI, "XXXX activate() entry scene='${scene.name}' id=${scene.id} duration=${durationMinutes}min actions=${scene.actions.size}")
 
         // Defensive precondition gate — covers automation rules and races
         // where UI state lagged behind reality. UI normally disables the entry
@@ -181,41 +180,24 @@ class SceneExecutor @Inject constructor(
         //   scheduleExpiryWorker handles any leftover work.
         if (activeSceneManager.isActive()) {
             val previouslyExpired = activeSceneManager.isExpired()
-            aapsLogger.info(LTag.UI, "XXXX activate() — winding down previous active scene '${activeSceneManager.getActiveState()?.scene?.name}' expired=$previouslyExpired")
             if (!previouslyExpired) {
                 deactivate()
             } else {
                 activeSceneManager.clearActive()
             }
         } else {
-            aapsLogger.info(LTag.UI, "XXXX activate() — no previous active scene")
         }
 
         val now = dateUtil.now()
         val durationMs = T.mins(durationMinutes.toLong()).msecs()
-        aapsLogger.info(LTag.UI, "XXXX activate() now=$now durationMs=$durationMs")
 
         // Capture pre-activation SMB flag (the only "truly prior" state) before making changes.
-        aapsLogger.info(LTag.UI, "XXXX activate() calling capturePriorSmb()")
-        val priorSmb = try {
-            capturePriorSmb(scene)
-        } catch (e: Throwable) {
-            aapsLogger.error(LTag.UI, "XXXX activate() capturePriorSmb FAILED", e)
-            throw e
-        }
-        aapsLogger.info(LTag.UI, "XXXX activate() capturePriorSmb() returned: $priorSmb")
+        val priorSmb = capturePriorSmb(scene)
 
         // Execute each action
         val actionResults = mutableListOf<SceneExecutionResult.ActionResult>()
-        for ((idx, action) in scene.actions.withIndex()) {
-            aapsLogger.info(LTag.UI, "XXXX activate() executing action $idx/${scene.actions.size}: ${action::class.simpleName}")
-            val result = try {
-                executeAction(action, durationMinutes, now)
-            } catch (e: Throwable) {
-                aapsLogger.error(LTag.UI, "XXXX activate() executeAction #$idx FAILED", e)
-                throw e
-            }
-            aapsLogger.info(LTag.UI, "XXXX activate() action $idx result: success=${result.success} err=${result.errorMessage}")
+        for (action in scene.actions) {
+            val result = executeAction(action, durationMinutes, now)
             actionResults.add(result)
         }
 
@@ -235,15 +217,12 @@ class SceneExecutor @Inject constructor(
             priorSmb = priorSmb,
             scopedRecords = scopedRecords
         )
-        aapsLogger.info(LTag.UI, "XXXX activate() setting active state for '${scene.name}'")
         activeSceneManager.setActive(activeState)
 
         // Schedule expiry notification if duration-based
         if (durationMs > 0) {
-            aapsLogger.info(LTag.UI, "XXXX activate() scheduling expiry worker in ${durationMs}ms")
             scheduleExpiryWorker(scene.name, durationMs)
         } else {
-            aapsLogger.info(LTag.UI, "XXXX activate() durationMs==0, no expiry worker scheduled (indefinite)")
         }
 
         // Log user entry
@@ -308,13 +287,10 @@ class SceneExecutor @Inject constructor(
      * Marks the scene as expired so the banner shows "Dismiss" instead of "End Scene".
      */
     suspend fun onExpiry() {
-        aapsLogger.info(LTag.UI, "XXXX onExpiry() entry")
         val activeState = activeSceneManager.getActiveState()
         if (activeState == null) {
-            aapsLogger.info(LTag.UI, "XXXX onExpiry() — no active state, returning")
             return
         }
-        aapsLogger.info(LTag.UI, "XXXX onExpiry() scene='${activeState.scene.name}' endAction=${activeState.scene.endAction}")
 
         val now = dateUtil.now()
 
@@ -330,14 +306,12 @@ class SceneExecutor @Inject constructor(
         // TT / LoopMode / CarePortal records self-expire via their own timestamp+duration queries.
         for (action in activeState.scene.actions) {
             if (action is SceneAction.SmbToggle || action is SceneAction.ProfileSwitch) {
-                aapsLogger.info(LTag.UI, "XXXX onExpiry() reverting ${action::class.simpleName}")
                 revertAction(action, activeState, now)
             }
         }
 
         // Mark as expired (keep state for banner display) instead of clearing.
         // Lifecycle change rides the existing RunningConfigurationPublisher cycle to clients.
-        aapsLogger.info(LTag.UI, "XXXX onExpiry() calling markExpired()")
         activeSceneManager.markExpired()
 
         // Log

implementation/src/main/kotlin/app/aaps/implementation/scenes/SceneExpiryWorker.kt

@@ -49,52 +49,39 @@ class SceneExpiryWorker @AssistedInject constructor(
             return Result.retry()
         }
         val sceneName = inputData.getString(KEY_SCENE_NAME) ?: "Scene"
-        aapsLogger.info(LTag.UI, "XXXX SceneExpiryWorker fired for '$sceneName'")
         val activeState = activeSceneManager.getActiveState()
         if (activeState == null) {
-            aapsLogger.info(LTag.UI, "XXXX no active state — worker exiting")
             return Result.success()
         }
         // If a previous run already expired this scene (e.g. WorkManager retried
         // after we returned Result.retry from the init gate), skip — re-running
         // onExpiry could double-activate a chained scene.
         if (activeSceneManager.isExpired()) {
-            aapsLogger.info(LTag.UI, "XXXX scene already expired — worker exiting")
             return Result.success()
         }
-        aapsLogger.info(LTag.UI, "XXXX active=${activeState.scene.name} id=${activeState.scene.id} endAction=${activeState.scene.endAction}")
 
         val endAction = activeState.scene.endAction
-        aapsLogger.info(LTag.UI, "XXXX calling onExpiry() — reverting non-duration actions")
         sceneExecutor.onExpiry()
-        aapsLogger.info(LTag.UI, "XXXX onExpiry() returned; activeState now=${activeSceneManager.getActiveState()?.scene?.name} expired=${activeSceneManager.isActive()}")
 
         if (endAction is SceneEndAction.ChainScene) {
-            aapsLogger.info(LTag.UI, "XXXX endAction is ChainScene → targetId=${endAction.sceneId}")
             runChain(sceneName, endAction.sceneId)
         } else {
-            aapsLogger.info(LTag.UI, "XXXX no chain configured (endAction=$endAction) → posting ended notification")
             postEndedNotification(sceneName)
         }
 
-        aapsLogger.info(LTag.UI, "XXXX SceneExpiryWorker finishing; final activeState=${activeSceneManager.getActiveState()?.scene?.name}")
         return Result.success()
     }
 
     private suspend fun runChain(endedName: String, targetId: String) {
-        aapsLogger.info(LTag.UI, "XXXX runChain from '$endedName' to targetId=$targetId")
         val target = sceneRepository.getScene(targetId)
         if (target == null) {
-            aapsLogger.info(LTag.UI, "XXXX target $targetId NOT FOUND in repository — skipping")
             postEndedWithSkip(endedName, rh.gs(R.string.scene_chain_skipped_deleted))
             return
         }
-        aapsLogger.info(LTag.UI, "XXXX target resolved: name='${target.name}' enabled=${target.isEnabled} actions=${target.actions.size} duration=${target.defaultDurationMinutes}min")
 
         val loopSuspended = loop.runningMode().pausesLoopExecution()
         val pumpInit = activePlugin.activePump.isInitialized()
         val profile = profileFunction.getProfile()
-        aapsLogger.info(LTag.UI, "XXXX preconditions: loopSuspended=$loopSuspended pumpInit=$pumpInit profile=${profile != null}")
 
         val skipReason: String? = when {
             !target.isEnabled            -> rh.gs(R.string.scene_chain_skipped_disabled, target.name)
@@ -104,21 +91,11 @@ class SceneExpiryWorker @AssistedInject constructor(
         }
 
         if (skipReason != null) {
-            aapsLogger.info(LTag.UI, "XXXX chain SKIPPED: $skipReason")
             postEndedWithSkip(endedName, skipReason)
             return
         }
 
-        aapsLogger.info(LTag.UI, "XXXX calling sceneExecutor.activate('${target.name}')")
-        val result = try {
-            sceneExecutor.activate(target)
-        } catch (e: Throwable) {
-            aapsLogger.error(LTag.UI, "XXXX sceneExecutor.activate('${target.name}') THREW", e)
-            throw e
-        }
-        aapsLogger.info(LTag.UI, "XXXX activate() returned: success=${result.success} error=${result.errorMessage}")
-        aapsLogger.info(LTag.UI, "XXXX actionResults: ${result.actionResults.joinToString { "${it.action::class.simpleName}=${it.success}${it.errorMessage?.let { e -> "($e)" } ?: ""}" }}")
-        aapsLogger.info(LTag.UI, "XXXX post-activate activeState=${activeSceneManager.getActiveState()?.scene?.name}")
+        val result = sceneExecutor.activate(target)
 
         if (result.success) {
             postChainSuccess(endedName, target.name)
@@ -127,7 +104,7 @@ class SceneExpiryWorker @AssistedInject constructor(
             val details = failed.joinToString("; ") {
                 "${it.action::class.simpleName}${it.errorMessage?.let { e -> ": $e" } ?: ""}"
             }
-            aapsLogger.error(LTag.UI, "XXXX chain '$endedName' → '${target.name}' partial failure — ${failed.size}/${result.actionResults.size} actions failed: $details")
+            aapsLogger.error(LTag.UI, "Scene chain '$endedName' → '${target.name}' partial failure — ${failed.size}/${result.actionResults.size} actions failed: $details")
             postChainError(endedName, target.name, failed.size, result.actionResults.size, details)
         }
     }

implementation/src/test/kotlin/app/aaps/implementation/scenes/SceneAutomationApiImplTest.kt

@@ -0,0 +1,58 @@
+package app.aaps.implementation.scenes
+
+import app.aaps.core.interfaces.notifications.NotificationManager
+import app.aaps.core.interfaces.resources.ResourceHelper
+import app.aaps.shared.tests.TestBase
+import com.google.common.truth.Truth.assertThat
+import org.junit.jupiter.api.Test
+import org.mockito.Mock
+import org.mockito.kotlin.whenever
+
+/**
+ * The two "is there a scene" questions and how they differ once a scene expires but its banner is
+ * still up — the state that used to block every automation gated on "no scene running" (#5059).
+ */
+class SceneAutomationApiImplTest : TestBase() {
+
+    @Mock lateinit var sceneRepository: SceneRepository
+    @Mock lateinit var sceneExecutor: SceneExecutor
+    @Mock lateinit var activeSceneManager: ActiveSceneManager
+    @Mock lateinit var sceneChainTargetResolver: SceneChainTargetResolver
+    @Mock lateinit var notificationManager: NotificationManager
+    @Mock lateinit var rh: ResourceHelper
+
+    private fun createSut() = SceneAutomationApiImpl(
+        sceneRepository, sceneExecutor, activeSceneManager, sceneChainTargetResolver, notificationManager, rh
+    )
+
+    private fun givenScene(active: Boolean, expired: Boolean) {
+        whenever(activeSceneManager.isActive()).thenReturn(active)
+        whenever(activeSceneManager.isExpired()).thenReturn(expired)
+    }
+
+    @Test
+    fun `a running scene is active and stoppable`() {
+        givenScene(active = true, expired = false)
+
+        assertThat(createSut().isAnySceneActive()).isTrue()
+        assertThat(createSut().hasSceneToStop()).isTrue()
+    }
+
+    @Test
+    fun `an expired scene is no longer active but is still stoppable`() {
+        // Its effects were reverted by onExpiry; only the "ended" banner is left. Automation gated on
+        // "no scene running" must be free to run, while the user (or the watch) can still dismiss it.
+        givenScene(active = true, expired = true)
+
+        assertThat(createSut().isAnySceneActive()).isFalse()
+        assertThat(createSut().hasSceneToStop()).isTrue()
+    }
+
+    @Test
+    fun `with no scene at all both answers are no`() {
+        givenScene(active = false, expired = false)
+
+        assertThat(createSut().isAnySceneActive()).isFalse()
+        assertThat(createSut().hasSceneToStop()).isFalse()
+    }
+}

plugins/sync/src/main/kotlin/app/aaps/plugins/sync/wear/wearintegration/DataHandlerMobile.kt

@@ -687,7 +687,9 @@ class DataHandlerMobile @Inject constructor(
     private fun handleSceneStopPreCheck() {
         // Build confirm locally — no master round-trip needed before showing "End active scene".
         // The watch waits for RemoteDelivered (deferConfirm) while the stop relays to master.
-        if (!scenes.isAnySceneActive()) return sendError(rh.gs(app.aaps.core.ui.R.string.scene_ended))
+        // Wider than "a scene is running": ending an expired-but-undismissed scene from the watch is a
+        // valid stop (stopActiveScene dismisses it), and it is the only remote way to clear that banner.
+        if (!scenes.hasSceneToStop()) return sendError(rh.gs(app.aaps.core.ui.R.string.scene_ended))
         sendToWear(
             EventData.ConfirmAction(
                 title = rh.gs(app.aaps.core.ui.R.string.scenes),
@@ -1101,7 +1103,9 @@ class DataHandlerMobile @Inject constructor(
         sendUserActions()
         // Scenes
         sendScenes()
-        sendActiveSceneState(scenes.isAnySceneActive())
+        // Same condition as the live push in WearPlugin (scenes.activeFlow) — the tile is fed by both,
+        // so a resend must not disagree with the flow about whether the STOP button belongs there.
+        sendActiveSceneState(scenes.hasSceneToStop())
         // GraphData
         iobCobCalculator.ads.getBucketedDataTableCopy()?.let { bucketedData ->
             // Hoist out of the per-bucket map: getGlucoseStatusData copies the bucketed table and runs a polynomial fit on every call.