Skip to content

Commit 7c72105

Browse files
fryanpanclaude
andcommitted
ADFA-4128 (11/11): address CodeRabbit review
- F1723-1 put QuickBuildPipelineTest into the suite that actually runs - F1723-4 guard the deferred prebuild fire() against a throw Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7
1 parent d648604 commit 7c72105

3 files changed

Lines changed: 40 additions & 1 deletion

File tree

‎app/src/androidTest/kotlin/com/itsaky/androidide/OrderedTestSuite.kt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import org.junit.runners.Suite
77
@Suite.SuiteClasses(
88
CleanupTest::class,
99
EndToEndTest::class,
10+
QuickBuildPipelineTest::class,
1011
QuickBuildSmokeTest::class,
1112
QuickBuildFlagOffTest::class,
1213
)

‎app/src/main/java/com/itsaky/androidide/quickbuild/QuickBuildPrebuildStagger.kt‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.itsaky.androidide.quickbuild
22

3+
import kotlinx.coroutines.CancellationException
34
import kotlinx.coroutines.CoroutineScope
45
import kotlinx.coroutines.Job
56
import kotlinx.coroutines.delay
@@ -66,7 +67,20 @@ class QuickBuildPrebuildStagger(
6667
scope.launch {
6768
delay(staggerMillis)
6869
synchronized(lock) { scheduled = null }
69-
fire()
70+
try {
71+
fire()
72+
} catch (e: CancellationException) {
73+
// Closing the project cancels this window; teardown has to stay cancellable.
74+
throw e
75+
} catch (e: Throwable) {
76+
// Nothing downstream catches this. The scope is the editor activity's, which
77+
// carries a plain Job and no CoroutineExceptionHandler, so a throw here takes
78+
// the IDE down half a minute after a project opens - with no action of the
79+
// user's in between - and short of that would cancel the scope for the life of
80+
// the activity, killing the editor's other launch sites with it. The immediate
81+
// fire() below is left alone: it runs on the caller's thread, which can handle it.
82+
log.error("Deferred Quick Build prebuild failed", e)
83+
}
7084
}
7185
}
7286
}

‎app/src/test/java/com/itsaky/androidide/quickbuild/QuickBuildPrebuildStaggerTest.kt‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package com.itsaky.androidide.quickbuild
33
import com.google.common.truth.Truth.assertThat
44
import kotlinx.coroutines.ExperimentalCoroutinesApi
55
import kotlinx.coroutines.cancel
6+
import kotlinx.coroutines.isActive
67
import kotlinx.coroutines.test.TestScope
78
import kotlinx.coroutines.test.advanceTimeBy
89
import kotlinx.coroutines.test.runCurrent
@@ -146,6 +147,29 @@ class QuickBuildPrebuildStaggerTest {
146147
assertThat(transition.effects).isEmpty()
147148
}
148149

150+
@Test
151+
fun `a deferred prebuild that throws does not take the scope down with it`() =
152+
runTest {
153+
stagger().onProjectSynced(
154+
sessionIsLive = { false },
155+
fire = { throw IllegalStateException("selectedVariantName blew up") },
156+
)
157+
158+
advanceTimeBy(STAGGER + 1)
159+
runCurrent()
160+
161+
// The scope is the editor activity's: a plain Job, no CoroutineExceptionHandler.
162+
// An escaping throw crashes the IDE outright, and short of that cancels the scope
163+
// for the life of the activity - taking the editor's other launch sites with it.
164+
assertThat(backgroundScope.isActive).isTrue()
165+
166+
// And the scope is still usable, not merely un-cancelled.
167+
stagger().onProjectSynced(sessionIsLive = { false }, fire = { fires++ })
168+
advanceTimeBy(STAGGER + 1)
169+
runCurrent()
170+
assertThat(fires).isEqualTo(1)
171+
}
172+
149173
companion object {
150174
private const val STAGGER = 30_000L
151175
}

0 commit comments

Comments
 (0)