Make robot discovery tolerate scope destruction

CHANGELOG.md

@@ -14,6 +14,7 @@
 
 ### Fixed
 
+- Skip scopes destroyed during robot lookup.
 - Allow the default Metro compiler integration to use `@ContributesBinding` on `Scoped` classes with an explicit qualified set binding.
 
 ### Security

robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt

@@ -81,7 +81,8 @@ public interface Robot {
  *
  * [rootScope] refers to the application scope. The scope and all of its children are searched for
  * the contributed robot. Usually, the parameter doesn't need to be changed and the default value
- * can be used.
+ * can be used. Scopes destroyed during discovery are skipped. Lookup does not keep scopes alive
+ * while the robot runs.
  */
 public inline fun <reified T : Robot> robot(
   rootScope: Scope = software.ralf.app.platform.robot.internal.rootScope,
@@ -103,39 +104,59 @@ public inline fun <reified T : Robot> robot(
 
 @PublishedApi
 internal fun Scope.findRobotFactory(robotType: KClass<*>): (() -> Robot)? {
-  var match: (() -> Robot)? = null
-  var matchingScope: Scope? = null
+  val matches = mutableMapOf<Scope, () -> Robot>()
 
   fun Scope.findRobot(inheritedRobotTypes: Set<KClass<*>>) {
-    if (isDestroyed()) return
+    if (isDestroyed()) {
+      return
+    }
 
-    val robots = allRobots
-    if (robotType !in inheritedRobotTypes) {
-      robots[robotType]?.let { robotFactory ->
-        check(match == null) {
-          "Found Robot of type $robotType in multiple scopes: " +
-            "${matchingScope?.name} and $name."
-        }
-        match = robotFactory
-        matchingScope = this
+    val component: RobotComponent?
+    val graph: RobotGraph?
+    val childScopes: Set<Scope>
+    try {
+      component = kotlinInjectComponentOrNull()
+      graph = metroDependencyGraphOrNull()
+      childScopes = children()
+    } catch (exception: IllegalStateException) {
+      // Destruction can finish after the initial check or between these reads.
+      if (isDestroyed()) {
+        return
       }
+      throw exception
+    }
+
+    val robots = robotFactories(component, graph)
+    if (robotType !in inheritedRobotTypes) {
+      robots[robotType]?.let { matches[this] = it }
     }
 
     val visibleRobotTypes = inheritedRobotTypes + robots.keys
-    children().forEach { childScope -> childScope.findRobot(visibleRobotTypes) }
+    childScopes.forEach { childScope -> childScope.findRobot(visibleRobotTypes) }
   }
 
   findRobot(emptySet())
-  return match
+
+  val liveMatches = matches.filterKeys { !it.isDestroyed() }
+  check(liveMatches.size <= 1) {
+    "Found Robot of type $robotType in multiple scopes: " +
+      "${liveMatches.keys.joinToString(" and ") { it.name }}."
+  }
+  return liveMatches.values.singleOrNull()
 }
 
 @PublishedApi
 internal val Scope.allRobots: Map<KClass<*>, () -> Robot>
   get() {
-    return kotlinInjectComponentOrNull<RobotComponent>()?.robots.orEmpty() +
-      metroDependencyGraphOrNull<RobotGraph>()?.robots.orEmpty().mapValues { { it.value() } }
+    return robotFactories(kotlinInjectComponentOrNull(), metroDependencyGraphOrNull())
   }
 
+private fun robotFactories(
+  component: RobotComponent?,
+  graph: RobotGraph?,
+): Map<KClass<*>, () -> Robot> =
+  component?.robots.orEmpty() + graph?.robots.orEmpty().mapValues { { it.value() } }
+
 private inline fun <reified T : Any> Scope.metroDependencyGraphOrNull(): T? {
   return try {
     metroDependencyGraph<T>()

robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt

@@ -5,6 +5,7 @@ import assertk.assertions.contains
 import assertk.assertions.isFalse
 import assertk.assertions.isNotNull
 import assertk.assertions.isNotSameInstanceAs
+import assertk.assertions.isSameInstanceAs
 import assertk.assertions.isTrue
 import kotlin.reflect.KClass
 import kotlin.test.Test
@@ -173,6 +174,202 @@ class RobotTest {
     assertFailsWith<IllegalStateException> { robot<ChildTestRobot>(rootScope) {} }
   }
 
+  @Test
+  fun `scopes destroyed before their services are read are skipped`() {
+    val expectedRobot = MetroTestRobot()
+    val rootScope = rootScope(kiRobot = null, metroRobot = expectedRobot)
+    val childScope = rootScope.buildChild("child")
+    val destroyingChild =
+      object : Scope by childScope {
+        override fun isDestroyed(): Boolean {
+          val destroyed = childScope.isDestroyed()
+          childScope.destroy()
+          return destroyed
+        }
+      }
+    val scope =
+      object : Scope by rootScope {
+        override fun children(): Set<Scope> = setOf(destroyingChild)
+      }
+
+    robot<MetroTestRobot>(scope) { assertThat(this).isSameInstanceAs(expectedRobot) }
+    assertThat(childScope.isDestroyed()).isTrue()
+  }
+
+  @Test
+  fun `scopes destroyed before their children are read are skipped`() {
+    val expectedRobot = MetroTestRobot()
+    val rootScope = rootScope(kiRobot = null, metroRobot = expectedRobot)
+    val childScope = rootScope.buildChild("child")
+    val destroyingChild =
+      object : Scope by childScope {
+        override fun children(): Set<Scope> {
+          childScope.destroy()
+          return childScope.children()
+        }
+      }
+    val scope =
+      object : Scope by rootScope {
+        override fun children(): Set<Scope> = setOf(destroyingChild)
+      }
+
+    robot<MetroTestRobot>(scope) { assertThat(this).isSameInstanceAs(expectedRobot) }
+    assertThat(childScope.isDestroyed()).isTrue()
+  }
+
+  @Test
+  fun `service failures from a live scope propagate unchanged`() {
+    val failure = IllegalStateException("Service lookup failed")
+    val scope =
+      object : Scope by rootScope() {
+        override fun <T : Any> getService(key: String): T? {
+          throw failure
+        }
+      }
+
+    val actual = assertFailsWith<IllegalStateException> { robot<MetroTestRobot>(scope) {} }
+
+    assertThat(actual).isSameInstanceAs(failure)
+    assertThat(scope.isDestroyed()).isFalse()
+  }
+
+  @Test
+  fun `children failures from a live scope propagate unchanged`() {
+    val failure = IllegalStateException("Children lookup failed")
+    val scope =
+      object : Scope by rootScope() {
+        override fun children(): Set<Scope> {
+          throw failure
+        }
+      }
+
+    val actual = assertFailsWith<IllegalStateException> { robot<MetroTestRobot>(scope) {} }
+
+    assertThat(actual).isSameInstanceAs(failure)
+    assertThat(scope.isDestroyed()).isFalse()
+  }
+
+  @Test
+  fun `scopes destroyed while reading robots do not hide a sibling robot`() {
+    val rootScope = rootScope(kiRobot = null, metroRobot = null)
+    lateinit var destroyingChild: Scope
+    destroyingChild =
+      rootScope.buildChild("destroying-child") {
+        addMetroDependencyGraph(
+          object : RobotGraph {
+            override val robots: Map<KClass<*>, () -> Robot>
+              get() {
+                destroyingChild.destroy()
+                return mapOf(ChildTestRobot::class to { ChildTestRobot() })
+              }
+          },
+        )
+      }
+    val expectedRobot = ChildTestRobot()
+    rootScope.buildChild("live-child") { addMetroDependencyGraph(Graph(expectedRobot)) }
+
+    robot<ChildTestRobot>(rootScope) { assertThat(this).isSameInstanceAs(expectedRobot) }
+    assertThat(destroyingChild.isDestroyed()).isTrue()
+  }
+
+  @Test
+  fun `a matching scope destroyed later during discovery is ignored`() {
+    val rootScope = rootScope(kiRobot = null, metroRobot = null)
+    val firstChild =
+      rootScope.buildChild("first-child") { addMetroDependencyGraph(Graph(ChildTestRobot())) }
+    val expectedRobot = ChildTestRobot()
+    rootScope.buildChild("second-child") {
+      addMetroDependencyGraph(
+        object : RobotGraph {
+          override val robots: Map<KClass<*>, () -> Robot>
+            get() {
+              firstChild.destroy()
+              return mapOf(ChildTestRobot::class to { expectedRobot })
+            }
+        },
+      )
+    }
+
+    robot<ChildTestRobot>(rootScope) { assertThat(this).isSameInstanceAs(expectedRobot) }
+    assertThat(firstChild.isDestroyed()).isTrue()
+  }
+
+  @Test
+  fun `robot getter failures propagate even if the scope was destroyed`() {
+    val rootScope = rootScope(kiRobot = null, metroRobot = null)
+    val failure = IllegalStateException("Robot getter failed")
+    lateinit var childScope: Scope
+    childScope =
+      rootScope.buildChild("child") {
+        addMetroDependencyGraph(
+          object : RobotGraph {
+            override val robots: Map<KClass<*>, () -> Robot>
+              get() {
+                childScope.destroy()
+                throw failure
+              }
+          },
+        )
+      }
+
+    val actual = assertFailsWith<IllegalStateException> { robot<ChildTestRobot>(rootScope) {} }
+
+    assertThat(actual).isSameInstanceAs(failure)
+  }
+
+  @Test
+  fun `robot getters accessing another destroyed scope propagate the failure`() {
+    val destroyedScope = Scope.buildRootScope("destroyed")
+    destroyedScope.destroy()
+    var failure: IllegalStateException? = null
+    val rootScope = Scope.buildRootScope {
+      addMetroDependencyGraph(
+        object : RobotGraph {
+          override val robots: Map<KClass<*>, () -> Robot>
+            get() =
+              try {
+                destroyedScope.getService<Map<KClass<*>, () -> Robot>>("robots").orEmpty()
+              } catch (exception: IllegalStateException) {
+                failure = exception
+                throw exception
+              }
+        },
+      )
+    }
+
+    val actual = assertFailsWith<IllegalStateException> { robot<ChildTestRobot>(rootScope) {} }
+
+    assertThat(actual).isSameInstanceAs(failure)
+    assertThat(rootScope.isDestroyed()).isFalse()
+  }
+
+  @Test
+  fun `robot factory failures propagate unchanged`() {
+    val failure = IllegalStateException("Robot factory failed")
+    val rootScope = Scope.buildRootScope {
+      addKotlinInjectComponent(
+        object : RobotComponent {
+          override val robots: Map<KClass<out Robot>, () -> Robot> =
+            mapOf(KiTestRobot::class to { throw failure })
+        },
+      )
+    }
+
+    val actual = assertFailsWith<IllegalStateException> { robot<KiTestRobot>(rootScope) {} }
+
+    assertThat(actual).isSameInstanceAs(failure)
+  }
+
+  @Test
+  fun `discovery starting at a child finds robots from its parent`() {
+    val kiRobot = KiTestRobot()
+    val metroRobot = MetroTestRobot()
+    val childScope = rootScope(kiRobot, metroRobot).buildChild("child")
+
+    robot<KiTestRobot>(childScope) { assertThat(this).isSameInstanceAs(kiRobot) }
+    robot<MetroTestRobot>(childScope) { assertThat(this).isSameInstanceAs(metroRobot) }
+  }
+
   private fun rootScope(
     kiRobot: Robot? = KiTestRobot(),
     metroRobot: Robot? = MetroTestRobot(),