fix(scene): address bot review comments on concurrency, cancellation, validation, and race conditions

2026-08-25 17:16:04 +08:00
parent db6a7978a9
commit 56451be0d8
11 changed files with 484 additions and 120 deletions
@@ -1,8 +1,10 @@
 package com.flagship.abox.manager.testing.fakes

+import com.flagship.abox.manager.domain.ConcurrencyMode
 import com.flagship.abox.manager.domain.DeviceName
 import com.flagship.abox.manager.domain.DomainError
 import com.flagship.abox.manager.domain.DomainResult
+import com.flagship.abox.manager.domain.ExecutionStrategy
 import com.flagship.abox.manager.domain.InfraredGateway
 import com.flagship.abox.manager.domain.ProviderProfileId
 import com.flagship.abox.manager.domain.Scene
@@ -18,9 +20,15 @@ import com.flagship.abox.manager.domain.SceneId
 import com.flagship.abox.manager.domain.SceneRepository
 import com.flagship.abox.manager.domain.SocketGateway
 import com.flagship.abox.manager.domain.SocketPowerState
+import java.util.concurrent.ConcurrentHashMap
+import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.async
+import kotlinx.coroutines.awaitAll
+import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.MutableStateFlow
-import java.util.concurrent.ConcurrentHashMap
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock

 /**
 * In-memory [SceneRepository] used by the scene UI and ViewModel tests. Scenes are isolated by
@@ -35,6 +43,10 @@ class FakeSceneRepository : SceneRepository {
        ProviderProfileId,
        MutableStateFlow<DomainResult<List<Scene>>>,
    >()
+    private val mutexes = ConcurrentHashMap<ProviderProfileId, Mutex>()
+
+    private fun mutexFor(profileId: ProviderProfileId): Mutex =
+        mutexes.getOrPut(profileId) { Mutex() }

    override fun observeScenes(
        profileId: ProviderProfileId,
@@ -51,10 +63,13 @@ class FakeSceneRepository : SceneRepository {
    override suspend fun createScene(
        profileId: ProviderProfileId,
        scene: Scene,
-    ): DomainResult<Scene> {
+    ): DomainResult<Scene> = mutexFor(profileId).withLock {
        require(scene.profileId == profileId) {
            "Scene profileId must match the supplied profileId"
        }
+        require(scene.actions.all { it.profileId == profileId }) {
+            "All scene actions must belong to the supplied provider profile"
+        }
        val scenes = currentScenes(profileId).orEmpty()
        if (scenes.any { it.id == scene.id }) {
            return DomainResult.Failure(
@@ -65,33 +80,36 @@ class FakeSceneRepository : SceneRepository {
        }
        val updated = scenes + scene
        publish(profileId, updated)
-        return DomainResult.Success(scene)
+        DomainResult.Success(scene)
    }

    override suspend fun updateScene(
        profileId: ProviderProfileId,
        scene: Scene,
-    ): DomainResult<Scene> {
+    ): DomainResult<Scene> = mutexFor(profileId).withLock {
        require(scene.profileId == profileId) {
            "Scene profileId must match the supplied profileId"
        }
+        require(scene.actions.all { it.profileId == profileId }) {
+            "All scene actions must belong to the supplied provider profile"
+        }
        val scenes = currentScenes(profileId).orEmpty()
        val index = scenes.indexOfFirst { it.id == scene.id }
        if (index < 0) return notFound(scene.id)
        val updated = scenes.toMutableList().apply { set(index, scene) }
        publish(profileId, updated)
-        return DomainResult.Success(scene)
+        DomainResult.Success(scene)
    }

    override suspend fun deleteScene(
        profileId: ProviderProfileId,
        sceneId: SceneId,
-    ): DomainResult<Unit> {
+    ): DomainResult<Unit> = mutexFor(profileId).withLock {
        val scenes = currentScenes(profileId).orEmpty()
        val updated = scenes.filterNot { it.id == sceneId }
        if (updated.size == scenes.size) return notFound(sceneId)
        publish(profileId, updated)
-        return DomainResult.Success(Unit)
+        DomainResult.Success(Unit)
    }

    /**
@@ -102,6 +120,9 @@ class FakeSceneRepository : SceneRepository {
        require(scenes.all { it.profileId == profileId }) {
            "All scenes must belong to the supplied provider profile"
        }
+        require(scenes.all { s -> s.actions.all { it.profileId == profileId } }) {
+            "All scene actions must belong to the supplied provider profile"
+        }
        publish(profileId, scenes)
        return this
    }
@@ -198,13 +219,43 @@ class FakeSceneExecutor(
            )
        }
        val startTime = System.currentTimeMillis()
-        val results = mutableListOf<SceneActionExecutionResult>()
+
        return try {
-            for (action in scene.actions) {
-                val actionStart = System.currentTimeMillis()
-                val execResult = executeAction(scene.profileId, action)
-                val duration = System.currentTimeMillis() - actionStart
-                results.add(execResult.copy(executionDurationMillis = duration))
+            val results: List<SceneActionExecutionResult> = if (scene.concurrencyMode == ConcurrencyMode.PARALLEL) {
+                coroutineScope {
+                    scene.actions.map { action ->
+                        async {
+                            val actionStart = System.currentTimeMillis()
+                            val execResult = executeAction(scene.profileId, action)
+                            val duration = System.currentTimeMillis() - actionStart
+                            execResult.copy(executionDurationMillis = duration)
+                        }
+                    }.awaitAll()
+                }
+            } else {
+                val sequentialResults = mutableListOf<SceneActionExecutionResult>()
+                var aborted = false
+                for (action in scene.actions) {
+                    if (aborted) {
+                        sequentialResults.add(
+                            SceneActionExecutionResult(
+                                action = action,
+                                outcome = SceneActionOutcome.SKIPPED,
+                                error = null,
+                            ),
+                        )
+                        continue
+                    }
+                    val actionStart = System.currentTimeMillis()
+                    val execResult = executeAction(scene.profileId, action)
+                    val duration = System.currentTimeMillis() - actionStart
+                    val enrichedResult = execResult.copy(executionDurationMillis = duration)
+                    sequentialResults.add(enrichedResult)
+                    if (enrichedResult.outcome == SceneActionOutcome.FAILURE && scene.strategy == ExecutionStrategy.ABORT_ON_ERROR) {
+                        aborted = true
+                    }
+                }
+                sequentialResults
            }
            val totalDuration = System.currentTimeMillis() - startTime
            DomainResult.Success(
@@ -214,6 +265,8 @@ class FakeSceneExecutor(
                    totalDurationMillis = totalDuration,
                ),
            )
+        } catch (e: CancellationException) {
+            throw e
        } finally {
            running.remove(scene.id)
        }
@@ -223,34 +276,44 @@ class FakeSceneExecutor(
        profileId: ProviderProfileId,
        action: SceneAction,
    ): SceneActionExecutionResult {
-        val result = when (action) {
-            is SceneAction.SocketPower -> socketGateway.setPower(
-                profileId = profileId,
-                deviceName = action.deviceName,
-                powerState = action.powerState,
-            )
+        return try {
+            val result = when (action) {
+                is SceneAction.SocketPower -> socketGateway.setPower(
+                    profileId = profileId,
+                    deviceName = action.deviceName,
+                    powerState = action.powerState,
+                )

-            is SceneAction.InfraredKey -> infraredGateway.sendKey(
-                profileId = profileId,
-                deviceName = action.deviceName,
-                key = action.key,
-            )
+                is SceneAction.InfraredKey -> infraredGateway.sendKey(
+                    profileId = profileId,
+                    deviceName = action.deviceName,
+                    key = action.key,
+                )

-            is SceneAction.VirtualRemoteButton,
-            is SceneAction.Delay,
-            is SceneAction.Notification -> DomainResult.Success(Unit)
-        }
-        return when (result) {
-            is DomainResult.Success -> SceneActionExecutionResult(
-                action = action,
-                outcome = SceneActionOutcome.SUCCESS,
-                error = null,
-            )
+                is SceneAction.VirtualRemoteButton,
+                is SceneAction.Delay,
+                is SceneAction.Notification -> DomainResult.Success(Unit)
+            }
+            when (result) {
+                is DomainResult.Success -> SceneActionExecutionResult(
+                    action = action,
+                    outcome = SceneActionOutcome.SUCCESS,
+                    error = null,
+                )

-            is DomainResult.Failure -> SceneActionExecutionResult(
+                is DomainResult.Failure -> SceneActionExecutionResult(
+                    action = action,
+                    outcome = SceneActionOutcome.FAILURE,
+                    error = result.error,
+                )
+            }
+        } catch (e: CancellationException) {
+            throw e
+        } catch (e: Exception) {
+            SceneActionExecutionResult(
                action = action,
                outcome = SceneActionOutcome.FAILURE,
-                error = result.error,
+                error = DomainError.OperationFailed(detail = e.message),
            )
        }
    }
@@ -11,6 +11,9 @@ import com.flagship.abox.manager.domain.SceneColor
 import com.flagship.abox.manager.domain.SceneIcon
 import com.flagship.abox.manager.domain.SceneId
 import com.flagship.abox.manager.domain.SocketPowerState
+import kotlinx.coroutines.async
+import kotlinx.coroutines.awaitAll
+import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.test.runTest
 import org.junit.Assert.assertEquals
@@ -122,6 +125,45 @@ class FakeSceneRepositoryTest {
        assertNotNull((value as DomainResult.Failure).error)
    }

+    @Test(expected = IllegalArgumentException::class)
+    fun createSceneRejectsMismatchedActionProfileId() = runTest {
+        val repo = FakeSceneRepository()
+        val crossAction = SceneAction.SocketPower(
+            id = SceneActionId("a-cross"),
+            profileId = ProviderProfileId("other"),
+            deviceName = DeviceName("socket-1"),
+            powerState = SocketPowerState.ON,
+        )
+        val scene = Scene(
+            id = SceneId("${localProfile.value}:scene-cross"),
+            profileId = localProfile,
+            name = "Cross Action",
+            actions = listOf(crossAction),
+        )
+        repo.createScene(localProfile, scene)
+    }
+
+    @Test
+    fun concurrentCreationsAreThreadSafe() = runTest {
+        val repo = FakeSceneRepository()
+        val count = 20
+        coroutineScope {
+            (1..count).map { idx ->
+                async {
+                    val scene = Scene(
+                        id = SceneId("${aboxProfile.value}:scene-$idx"),
+                        profileId = aboxProfile,
+                        name = "Scene $idx",
+                        actions = emptyList(),
+                    )
+                    repo.createScene(aboxProfile, scene)
+                }
+            }.awaitAll()
+        }
+        val scenes = scenesFor(repo, aboxProfile)
+        assertEquals(count, scenes.size)
+    }
+
    private suspend fun scenesFor(
        repo: FakeSceneRepository,
        profileId: ProviderProfileId,