ADFA-4128: qb 08 review fixes — cancel sequencing, provision tail, deploy-throw containment

Stale cancel flag: the Prebuilding stop latches proxyAppBuildCancelIssued with no teardown to clear it, so a later "Restart session" skipped the Gradle cancel -> clear the flag whenever an effect launches new session work (StartProvisioning / StartProxyAppPrebuild / RunProxyAppRebuild); covered by "a session started after a prebuild-stop still gets its Gradle build cancelled on restart".

Unguarded provision-success tail: retention clear, generation adoption and watcher.start ran unguarded on a scope with no CoroutineExceptionHandler -> wrap the tail in the same try/catch -> ProvisioningFailed boundary the rebuild arm already uses; covered by "a watcher-start throw in provisioning's success tail fails the session instead of escaping".

Collector-killing deploy throw: resendRetainedPayload called deploy.deploy() bare inside the init-launched reconnect collector, so one throw disabled catch-up for the process -> contain non-cancellation throwables as a failed re-send (return false, fall back to the catch-up build); covered by "a throwing re-send is contained - catch-up falls back now and stays alive for later reconnects".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj9YeCDHGp9DU8LPtfWJ7W

Author: fryanpan

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/service/session/QuickBuildSessionManager.kt

@@ -259,8 +259,10 @@ class QuickBuildSessionManager(
 	 * Whether [SessionEffect.CancelProxyAppBuild] already cancelled this session's Gradle build,
 	 * so [teardown] does not ask a second time. The stop-tap path emits that effect and a
 	 * teardown; every OTHER teardown (a restart, an invalidation, a project close) emits only the
-	 * teardown, which is the case teardown's own cancel exists for. Cleared in [teardown], which
-	 * always follows the effect.
+	 * teardown, which is the case teardown's own cancel exists for. Cleared in [teardown], and
+	 * again whenever an effect launches new session work: the Prebuilding stop drops a queued
+	 * tap with the cancel effect but NO teardown, and left latched the flag would make the next
+	 * session's teardown skip the cancel of a Gradle build it never covered.
 	 */
 	private var proxyAppBuildCancelIssued = false
 
@@ -654,11 +656,16 @@ class QuickBuildSessionManager(
 	private fun runEffect(effect: SessionEffect) {
 		when (effect) {
 			SessionEffect.StartProvisioning -> {
+				// A cancel issued against a previous build does not cover the one starting
+				// here; see [proxyAppBuildCancelIssued] for the Prebuilding stop that
+				// latches it with no teardown to clear it.
+				proxyAppBuildCancelIssued = false
 				val epoch = sessionEpoch
 				sessionWork = scope.launch { provision(epoch) }
 			}
 
 			SessionEffect.StartProxyAppPrebuild -> {
+				proxyAppBuildCancelIssued = false
 				sessionWork = scope.launch { runPrebuild() }
 			}
 
@@ -718,6 +725,7 @@ class QuickBuildSessionManager(
 			}
 
 			SessionEffect.RunProxyAppRebuild -> {
+				proxyAppBuildCancelIssued = false
 				val epoch = sessionEpoch
 				sessionWork = scope.launch { rebuildProxyApp(epoch) }
 			}
@@ -964,22 +972,35 @@ class QuickBuildSessionManager(
 				live = result.session
 				staleComponentHelpersNoticed = false
 				testSourceIgnoredNoticed = false
-				// A same-project predecessor's scratch tree can survive its teardown (see
-				// [teardown]'s skip when a new session went live mid-shutdown); whatever it
-				// retained belongs to another baseline and must not answer this session's
-				// reconnects.
-				result.session.retainedPayloads.clear()
-				// The installed APK boots at the stamped baseline generation (concurrency.md
-				// rule 2): the allocator must stay strictly above it, and adopting it as the
-				// deploy tally makes a reconnect at the stamp read in-sync by construction.
-				result.tracker.adoptAtLeast(result.baselineGeneration)
-				result.session.lastDeployedGeneration = result.baselineGeneration
-				// Build ids restart per session; give the sink its session boundary.
-				report { metrics.onSessionStarted() }
-				// The reload path is change-driven, not save-driven: any source of a
-				// file change triggers it, including Termux, plugins and git.
-				result.session.watcher.start(::onWatcherBatch)
-				dispatch(SessionEvent.ProvisioningSucceeded(result.baselineGeneration))
+				try {
+					// A same-project predecessor's scratch tree can survive its teardown (see
+					// [teardown]'s skip when a new session went live mid-shutdown); whatever it
+					// retained belongs to another baseline and must not answer this session's
+					// reconnects.
+					result.session.retainedPayloads.clear()
+					// The installed APK boots at the stamped baseline generation (concurrency.md
+					// rule 2): the allocator must stay strictly above it, and adopting it as the
+					// deploy tally makes a reconnect at the stamp read in-sync by construction.
+					result.tracker.adoptAtLeast(result.baselineGeneration)
+					result.session.lastDeployedGeneration = result.baselineGeneration
+					// Build ids restart per session; give the sink its session boundary.
+					report { metrics.onSessionStarted() }
+					// The reload path is change-driven, not save-driven: any source of a
+					// file change triggers it, including Termux, plugins and git.
+					result.session.watcher.start(::onWatcherBatch)
+					dispatch(SessionEvent.ProvisioningSucceeded(result.baselineGeneration))
+				} catch (e: kotlinx.coroutines.CancellationException) {
+					throw e
+				} catch (e: Throwable) {
+					// The runner's error boundary ends at its outcome; this tail (retention
+					// IO, the persisted generation store, the FileObserver registration) is
+					// the manager's half of the same assembly, and a throw here would escape
+					// to a scope with no CoroutineExceptionHandler and crash CoGo with the
+					// daemon up and the uid session registered. [live] is already set, so the
+					// failure effect's teardown unwinds both.
+					log.error("Installing the provisioned quick-build session threw", e)
+					dispatch(SessionEvent.ProvisioningFailed(QuickBuildMessage.Literal(e.message ?: e.javaClass.name)))
+				}
 			}
 		}
 	}
@@ -1268,13 +1289,29 @@ class QuickBuildSessionManager(
 			retained.generation,
 		)
 		val result =
-			deploy.deploy(
-				retained.generation,
-				retained.dexFile,
-				retained.arscFile,
-				retained.assetsZip,
-				retained.metadataJson,
-			)
+			try {
+				deploy.deploy(
+					retained.generation,
+					retained.dexFile,
+					retained.arscFile,
+					retained.assetsZip,
+					retained.metadataJson,
+				)
+			} catch (e: kotlinx.coroutines.CancellationException) {
+				throw e
+			} catch (e: Throwable) {
+				// deploy() is throw-capable (see notifyBuilding's guard), and this runs
+				// inside the reconnect collector launched once in [init]: an escaping
+				// throw would kill that collector for the rest of the process, and every
+				// later stale reconnect would run old code silently - the exact failure
+				// the collector exists to prevent. Contain it as a failed re-send.
+				log.warn(
+					"Re-send of retained generation {} threw; falling back to a catch-up build",
+					retained.generation,
+					e,
+				)
+				return false
+			}
 		if (result is DeployResult.Reloaded) return true
 		log.warn(
 			"Re-send of retained generation {} failed ({}); falling back to a catch-up build",

quickbuild/core/src/test/java/org/appdevforall/cotg/quickbuild/service/Fakes.kt

@@ -150,6 +150,12 @@ class FakeDeploy : DeploySender {
 	val resultQueue = ArrayDeque<DeployResult>()
 	var disconnects: Boolean = true
 
+	/**
+	 * When set, every [deploy] throws it after recording the call. Stands in for a binder
+	 * edge the sender did not classify into a [DeployResult] - the contract is throw-capable.
+	 */
+	var deployError: Throwable? = null
+
 	/**
 	 * Generation the fake "relaunched app" reconnects at, given the last deployed
 	 * generation; return null for a relaunch that never reconnects. Defaults to a
@@ -165,6 +171,7 @@ class FakeDeploy : DeploySender {
 		metadataJson: String,
 	): DeployResult {
 		calls += Call(generation, dexFile, arscFile, assetsZip, metadataJson)
+		deployError?.let { throw it }
 		return resultQueue.removeFirstOrNull() ?: result
 	}
 

quickbuild/core/src/test/java/org/appdevforall/cotg/quickbuild/service/session/QuickBuildSessionManagerTest.kt

@@ -167,6 +167,12 @@ class QuickBuildSessionManagerTest {
 	/** Captures the watcher the manager builds so a test can push change batches. */
 	private var watcher: FakeWatcher? = null
 
+	/**
+	 * When set, the manager-built watcher's [ProjectWatcher.start] throws it. Stands in for a
+	 * real FileObserver/inotify registration failure in provisioning's success tail.
+	 */
+	private var watcherStartError: (() -> Throwable)? = null
+
 	/**
 	 * Every request to bring the proxy app to the foreground, as (package, launcherActivity).
 	 * Behaviours 2/3/4 are exactly "is this list empty, and when did it grow", so it is the
@@ -196,13 +202,15 @@ class QuickBuildSessionManagerTest {
 	 */
 	private class FakeWatcher(
 		private val filter: WatchFilter,
+		private val startError: () -> Throwable? = { null },
 	) : ProjectWatcher {
 		private var onBatch: ((ChangedFiles.Known) -> Unit)? = null
 
 		/** Survives [stop]; see [emitRacingStop]. */
 		private var lastOnBatch: ((ChangedFiles.Known) -> Unit)? = null
 
 		override fun start(onBatch: (ChangedFiles.Known) -> Unit) {
+			startError()?.let { throw it }
 			this.onBatch = onBatch
 			this.lastOnBatch = onBatch
 		}
@@ -349,7 +357,9 @@ class QuickBuildSessionManagerTest {
 					}
 				}
 			},
-			watcherFactory = { _, _, filter, _ -> FakeWatcher(filter).also { watcher = it } },
+			watcherFactory = { _, _, filter, _ ->
+				FakeWatcher(filter, { watcherStartError?.invoke() }).also { watcher = it }
+			},
 			metrics = recordingMetrics,
 			warmCompileEnabled = warmCompileEnabled,
 			nowMillis = nowMillis,
@@ -2220,6 +2230,39 @@ class QuickBuildSessionManagerTest {
 			assertThat(executed.last().forced).isTrue()
 		}
 
+	@Test
+	fun `a throwing re-send is contained - catch-up falls back now and stays alive for later reconnects`() =
+		runTest {
+			// deploy() is throw-capable, and the re-send runs inside the reconnect
+			// collector launched once in init: an escaping throw would kill that
+			// collector for the rest of the process, and every later stale reconnect
+			// would run old code silently - the exact failure it exists to prevent.
+			val manager = createManager()
+			manager.onQuickBuildTapped()
+			advanceUntilIdle()
+			manager.save(sourceFile)
+			advanceUntilIdle()
+			assertThat(executed).hasSize(1)
+
+			seedRetainedPayload(generation = 1L)
+			deploy.deployError = RuntimeException("binder transaction failed")
+			connections.onConnected(connectedAt(0))
+			advanceUntilIdle()
+
+			// Contained like any other failed re-send: attempted once, then the
+			// last-resort forced rebuild of current sources.
+			assertThat(deploy.calls).hasSize(1)
+			assertThat(executed).hasSize(2)
+			assertThat(executed.last().forced).isTrue()
+
+			// The collector survived: the next stale reconnect is still repaired.
+			deploy.deployError = null
+			connections.onConnected(connectedAt(0))
+			advanceUntilIdle()
+			assertThat(executed).hasSize(3)
+			assertThat(executed.last().forced).isTrue()
+		}
+
 	@Test
 	fun `retention from an older deploy is never replayed - the forced build repairs instead`() =
 		runTest {
@@ -3830,6 +3873,45 @@ class QuickBuildSessionManagerTest {
 			assertThat(manager.state.value).isEqualTo(QuickBuildSessionState.Idle())
 		}
 
+	@Test
+	fun `a session started after a prebuild-stop still gets its Gradle build cancelled on restart`() =
+		runTest {
+			// The Prebuilding stop above latches the cancel-issued flag with no teardown to
+			// clear it. Left stale, the NEXT session's teardown would skip the Gradle
+			// cancel, the orphaned build would keep the device's one build slot, and the
+			// user's "Restart session" would come back as a SlotBusy setup failure.
+			prebuildGate = CompletableDeferred()
+			val manager = createManager()
+			manager.prebuild()
+			advanceUntilIdle()
+			manager.onQuickBuildTapped()
+			advanceUntilIdle()
+			manager.onCancelRequested()
+			advanceUntilIdle()
+			assertThat(proxyAppBuildCancelCount).isEqualTo(1)
+			assertThat(manager.state.value).isEqualTo(QuickBuildSessionState.Idle())
+			prebuildGate!!.complete(Unit)
+			advanceUntilIdle()
+
+			// A fresh tap owns a fresh Gradle proxy app build...
+			provisionGate = CompletableDeferred()
+			manager.onQuickBuildTapped()
+			advanceUntilIdle()
+			assertThat(manager.state.value)
+				.isEqualTo(QuickBuildSessionState.Provisioning(userInitiated = true))
+
+			// ...so the restart's teardown must reach the Gradle cancel: nothing else
+			// releases the build slot for the reprovision it goes on to run.
+			manager.restartSessionAndReprovision()
+			advanceUntilIdle()
+			assertThat(proxyAppBuildCancelCount).isEqualTo(2)
+
+			// And the reprovision itself still lands.
+			provisionGate!!.complete(Unit)
+			advanceUntilIdle()
+			assertThat(manager.state.value).isEqualTo(QuickBuildSessionState.Ready(0))
+		}
+
 	@Test
 	fun `stopping during provisioning cancels the proxy app build and tears the session down`() =
 		runTest {
@@ -3856,6 +3938,33 @@ class QuickBuildSessionManagerTest {
 			assertThat(watcher).isNull()
 		}
 
+	@Test
+	fun `a watcher-start throw in provisioning's success tail fails the session instead of escaping`() =
+		runTest {
+			// The install tail after a successful provision (retention clear, generation
+			// adoption, watcher registration) runs on a scope with no
+			// CoroutineExceptionHandler: an escaping throw would crash CoGo with the
+			// daemon up and the uid session registered, and strand the machine in
+			// Provisioning.
+			watcherStartError = { IllegalStateException("inotify watch limit reached") }
+			val manager = createManager()
+			manager.onQuickBuildTapped()
+			advanceUntilIdle()
+
+			// Same path as any other failed provision: torn down clean to Idle with the
+			// error surfaced and the daemon down - never a crash or a wedged Provisioning.
+			assertThat(manager.state.value).isEqualTo(QuickBuildSessionState.Idle(lastStartFailed = true))
+			assertThat(userMessages).contains(QuickBuildMessage.Literal("inotify watch limit reached"))
+			assertThat(daemon.isRunning).isFalse()
+
+			// The next tap re-provisions from scratch - not wedged.
+			watcherStartError = null
+			manager.onQuickBuildTapped()
+			advanceUntilIdle()
+			assertThat(provisionCount).isEqualTo(2)
+			assertThat(manager.state.value).isEqualTo(QuickBuildSessionState.Ready(0))
+		}
+
 	/**
 	 * A provision whose baseline declares [components] - the only fact the stale-helper
 	 * warning keys on, since whether such a component is currently INSTANTIATED is unknowable