From efb595ac3834c2fcda6ab3edbdc807d2198f5c65 Mon Sep 17 00:00:00 2001 From: Ralf Wondratschek Date: Fri, 16 Oct 2026 12:44:38 -0700 Subject: [PATCH] Make robot discovery tolerate scope destruction Robot lookup can overlap scope cleanup after navigation and fail while reading services or children. Skip those failures when the scope has been destroyed, and discard destroyed candidates before checking for ambiguity. Preserve ancestor robot lookup and propagate failures from live scopes, robot getters, and robot factories unchanged. Keep the fix in `:robot:public` without changing `:scope` or its synchronization. Lookup does not extend a scope's lifetime. --- CHANGELOG.md | 1 + .../software/ralf/app/platform/robot/Robot.kt | 55 +++-- .../ralf/app/platform/robot/RobotTest.kt | 197 ++++++++++++++++++ 3 files changed, 236 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d936ab0..1966238c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt b/robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt index bb1af4f4..c4e6ca54 100644 --- a/robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt +++ b/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 robot( rootScope: Scope = software.ralf.app.platform.robot.internal.rootScope, @@ -103,39 +104,59 @@ public inline fun robot( @PublishedApi internal fun Scope.findRobotFactory(robotType: KClass<*>): (() -> Robot)? { - var match: (() -> Robot)? = null - var matchingScope: Scope? = null + val matches = mutableMapOf Robot>() fun Scope.findRobot(inheritedRobotTypes: Set>) { - 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 + 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, () -> Robot> get() { - return kotlinInjectComponentOrNull()?.robots.orEmpty() + - metroDependencyGraphOrNull()?.robots.orEmpty().mapValues { { it.value() } } + return robotFactories(kotlinInjectComponentOrNull(), metroDependencyGraphOrNull()) } +private fun robotFactories( + component: RobotComponent?, + graph: RobotGraph?, +): Map, () -> Robot> = + component?.robots.orEmpty() + graph?.robots.orEmpty().mapValues { { it.value() } } + private inline fun Scope.metroDependencyGraphOrNull(): T? { return try { metroDependencyGraph() diff --git a/robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt b/robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt index 7bac60d4..3b558223 100644 --- a/robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt +++ b/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 { robot(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 = setOf(destroyingChild) + } + + robot(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 { + childScope.destroy() + return childScope.children() + } + } + val scope = + object : Scope by rootScope { + override fun children(): Set = setOf(destroyingChild) + } + + robot(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 getService(key: String): T? { + throw failure + } + } + + val actual = assertFailsWith { robot(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 { + throw failure + } + } + + val actual = assertFailsWith { robot(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, () -> Robot> + get() { + destroyingChild.destroy() + return mapOf(ChildTestRobot::class to { ChildTestRobot() }) + } + }, + ) + } + val expectedRobot = ChildTestRobot() + rootScope.buildChild("live-child") { addMetroDependencyGraph(Graph(expectedRobot)) } + + robot(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, () -> Robot> + get() { + firstChild.destroy() + return mapOf(ChildTestRobot::class to { expectedRobot }) + } + }, + ) + } + + robot(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, () -> Robot> + get() { + childScope.destroy() + throw failure + } + }, + ) + } + + val actual = assertFailsWith { robot(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, () -> Robot> + get() = + try { + destroyedScope.getService, () -> Robot>>("robots").orEmpty() + } catch (exception: IllegalStateException) { + failure = exception + throw exception + } + }, + ) + } + + val actual = assertFailsWith { robot(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, () -> Robot> = + mapOf(KiTestRobot::class to { throw failure }) + }, + ) + } + + val actual = assertFailsWith { robot(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(childScope) { assertThat(this).isSameInstanceAs(kiRobot) } + robot(childScope) { assertThat(this).isSameInstanceAs(metroRobot) } + } + private fun rootScope( kiRobot: Robot? = KiTestRobot(), metroRobot: Robot? = MetroTestRobot(),