ADFA-4128: qb 06 review fixes — binder-death handling, reconnect atomicity, deploy teardown

- Swallowed linkToDeath failure -> a binder dead at connect is reported as an
  instant death and never registered, so deploys fail fast as NotConnected
  instead of timing out with the freezer hold kept on a dead package (tests:
  "a binder that is dead at connect is not left registered", "a dead binder's
  stale connect retry does not clobber a live registration").
- Non-atomic connect watch/registration -> registration and death watch are one
  @synchronized step, and death delivery shares the lock, so the watched binder
  and the registered target can never disagree and a death cannot slip between
  link and registration (test: "a death delivered while connect is registering
  still clears the target"; wiring: "a reconnect moves the watch, and firing it
  clears the registration").
- Restart payload retained with hot-swap metadata -> a confirmed restart deploy
  clears the retained set instead of retaining it, so a reconnect catch-up can
  never hot-swap over the live restart-sensitive component and falls back to
  the forced rebuild (test: "a confirmed restart deploy clears the retained
  payload instead of retaining it").
- endSession leaving an in-flight deploy to time out -> endSession routes
  through onDisconnected, whose Disconnected report answers the waiter
  deterministically (test: "ending the session answers a deploy awaiting its
  verdict as Disconnected").
- Adjacent minor: disconnect() now unlinks the death watch, so no stale
  recipient outlives a graceful disconnect (test: "a graceful disconnect
  unlinks the death watch").

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/deploy/PayloadDeployer.kt

@@ -257,9 +257,15 @@ internal class PayloadDeployer(
 			}
 
 			else -> {
-				// Retained with hot-swap metadata, not this deploy's restart flag: a
-				// reconnect catch-up must not ask the just-relaunched app to exit again.
-				retention?.retain(generation, dexFile, arscFile, assets?.zip, metadata(restart = false))
+				// Retain nothing, and drop what is retained: the reconnect catch-up replays
+				// retained bytes as a hot swap, and hot-swapping a code-bearing payload onto
+				// a process holding the live component this deploy restarted for recreates
+				// the cross-loader ClassCastException the restart existed to prevent (see
+				// DeployPolicy). Retaining it with the restart flag is no better - the
+				// re-send has no relaunch behind it, so the app would exit and stay closed.
+				// A below-deployed reconnect after this deploy falls back to the forced
+				// catch-up rebuild, which re-derives the route.
+				retention?.clear()
 				// t3: the relaunched process reconnected at the deployed generation, so
 				// the restart swap is live. Slower than a hot swap by a full process
 				// launch. One clock read feeds both, as on the hot-swap path.

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/service/deploy/ProxyAppConnections.kt

@@ -91,10 +91,12 @@ class ProxyAppConnections {
 	fun endSession() {
 		expectedPackage = null
 		expectedUid = null
-		_target.value = null
-		// Nothing can deploy to the app now, so stop exempting it from the freezer: a hold
-		// kept past session end would cost the user battery on an app they are just running.
-		priorityHold?.release()
+		// Routed through [onDisconnected] so its Disconnected report answers a deploy still
+		// awaiting its verdict - the session's end settles that deploy now, instead of
+		// leaving it to ride out its full timeout against a target that is already gone.
+		// The unconditional drop also releases the freezer hold: a hold kept past session
+		// end would cost the user battery on an app they are just running.
+		onDisconnected()
 	}
 
 	/**

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/service/deploy/QuickBuildHostService.kt

@@ -65,10 +65,8 @@ class QuickBuildHostService : Service() {
 				throw SecurityException("connect() with null target or packageName")
 			}
 
-			watchForDeath(target.asBinder())
-
 			log.info("Proxy app {} connected at generation {}", packageName, runningGeneration)
-			connections.onConnected(ConnectedTarget(target, packageName, runningGeneration))
+			register(ConnectedTarget(target, packageName, runningGeneration))
 		}
 
 		override fun reportReloaded(
@@ -88,30 +86,70 @@ class QuickBuildHostService : Service() {
 		}
 
 		/**
-		 * Points the death watch at [binder], dropping the watch a superseded process left.
+		 * Registers [connection] and points the death watch at its binder, as one atomic step.
 		 *
 		 * Clearing the registration on death is what makes a deploy into a dead proxy app fail
 		 * fast as NotConnected instead of timing out on its binder. The unlink matters because a
 		 * recipient would otherwise accumulate one per reconnect, and the binder is passed on so
 		 * a late death from a superseded process cannot wipe the live registration.
 		 *
-		 * @param binder the connecting target's binder; null only for a local (non-binder) target,
-		 *   which cannot die out from under us and so needs no watch
+		 * One synchronized step, not watch-then-register: two racing connects could otherwise
+		 * interleave so the registered target and the watched binder disagree, and a dead target
+		 * would then stay registered with no death notification ever coming. [onBinderDeath]
+		 * shares the lock for the same reason, so a death delivered while a connect is
+		 * mid-registration lands after the registration it must clear, never before it.
 		 */
 		@Synchronized
-		private fun watchForDeath(binder: IBinder?) {
-			if (binder == null) return
+		private fun register(connection: ConnectedTarget) {
+			// Null only for a local (non-binder) target, which cannot die out from under us.
+			val binder = connection.target.asBinder()
+			if (binder == null) {
+				clearDeathWatch()
+				connections.onConnected(connection)
+				return
+			}
+			val recipient = IBinder.DeathRecipient { onBinderDeath(binder) }
+			try {
+				binder.linkToDeath(recipient, 0)
+			} catch (e: Exception) {
+				// Already dead at connect time: registering it would hold a dead target no
+				// death notification can ever clear, so every deploy would ride out its
+				// timeout with the freezer hold kept on a dead package. Report an instant
+				// death instead, and keep any previous watch - the superseded-binder guard
+				// in [ProxyAppConnections.onDisconnected] protects a still-live registration.
+				log.warn("Proxy app {} died before its connect completed", connection.packageName, e)
+				connections.onDisconnected(binder)
+				return
+			}
+			clearDeathWatch()
+			deathWatch = binder to recipient
+			connections.onConnected(connection)
+		}
+
+		/**
+		 * Handles a watched binder's death. Shares [register]'s lock so a death cannot be
+		 * consumed between the link and the registration it should clear.
+		 */
+		@Synchronized
+		private fun onBinderDeath(binder: IBinder) {
+			connections.onDisconnected(binder)
+		}
+
+		/** Drops the current death watch, unlinking its recipient. */
+		@Synchronized
+		private fun clearDeathWatch() {
 			deathWatch?.let { (previous, recipient) ->
 				runCatching { previous.unlinkToDeath(recipient, 0) }
 			}
-			val recipient = IBinder.DeathRecipient { connections.onDisconnected(binder) }
-			deathWatch = binder to recipient
-			runCatching { binder.linkToDeath(recipient, 0) }
+			deathWatch = null
 		}
 
 		override fun disconnect(packageName: String?) {
 			enforceCaller("disconnect")
 			log.info("Proxy app {} disconnected", packageName)
+			// Unlink too: a stale recipient would otherwise fire on the process's eventual
+			// death and report a disconnect against whatever is registered by then.
+			clearDeathWatch()
 			connections.onDisconnected()
 		}
 

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/service/deploy/RetainedPayloadStore.kt

@@ -29,8 +29,10 @@ internal class RetainedPayloadStore(
 	 * @property generation the generation the deploy claimed; a re-send replays it unchanged,
 	 *   and the runtime's strictly-newer gate accepts it because the reconnected app runs
 	 *   something older
-	 * @property metadataJson metadata for the re-send; always the hot-swap variant, since a
-	 *   reconnect catch-up must not ask the just-relaunched app to persist and exit again
+	 * @property metadataJson metadata for the re-send; always the hot-swap variant, because
+	 *   only hot-swap deploys are retained - a restart deploy [clear]s the store instead,
+	 *   since replaying its payload as a hot swap would land on the live restart-sensitive
+	 *   component the restart existed to protect
 	 * @property dexFile the retained classes, or null when the deploy carried none
 	 * @property arscFile the retained resource APK, or null when the deploy carried none
 	 * @property assetsZip the retained changed-assets zip, or null when the deploy carried none
@@ -117,8 +119,9 @@ internal class RetainedPayloadStore(
 	}
 
 	/**
-	 * Drops the retained set. Call whenever the baseline changes: the old baseline's bytes
-	 * must never be replayed onto a new one.
+	 * Drops the retained set. Call whenever the baseline changes - the old baseline's bytes
+	 * must never be replayed onto a new one - and after a confirmed restart deploy, whose
+	 * generation supersedes the retained one but must never be replayed as a hot swap.
 	 */
 	fun clear() {
 		dir.deleteRecursively()

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

@@ -122,6 +122,21 @@ class DeployChannelDeployTest {
 			assertThat(deploy.await()).isEqualTo(DeployResult.Disconnected)
 		}
 
+	@Test
+	fun `ending the session answers a deploy awaiting its verdict as Disconnected`() =
+		runTest {
+			val target = ScriptedTarget()
+			connect(target)
+
+			val deploy = async { channel.deploy(3, null, null, null, "{}") }
+			runCurrent()
+			connections.endSession()
+
+			// Session teardown settles the in-flight deploy now; without the Disconnected
+			// report it would ride out the full 5 s timeout and read as TimedOut.
+			assertThat(deploy.await()).isEqualTo(DeployResult.Disconnected)
+		}
+
 	@Test
 	fun `a binder failure during onPayload reports Failed naming the binder`() =
 		runTest {

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

@@ -16,9 +16,10 @@ import org.junit.jupiter.api.io.TempDir
 import java.io.File
 
 /**
- * Retention side of [PayloadDeployer] (concurrency.md rules 3-4): a deploy the proxy app
- * confirmed leaves its bytes in the [RetainedPayloadStore] for the reconnect re-send, and an
- * unconfirmed one leaves the store exactly as it was.
+ * Retention side of [PayloadDeployer] (concurrency.md rules 3-4): a confirmed hot-swap
+ * deploy leaves its bytes in the [RetainedPayloadStore] for the reconnect re-send, an
+ * unconfirmed one leaves the store exactly as it was, and a confirmed restart deploy clears
+ * it - its payload must never be replayed as a hot swap.
  */
 class PayloadDeployerRetentionTest {
 	@TempDir lateinit var workDir: File
@@ -120,26 +121,35 @@ class PayloadDeployerRetentionTest {
 		}
 
 	@Test
-	fun `a confirmed restart deploy retains hot-swap metadata, not the restart flag`() =
+	fun `a confirmed restart deploy clears the retained payload instead of retaining it`() =
 		runTest {
-			deploy.result = DeployResult.Reloaded(40)
+			val deployer = deployer()
+			deployer.deploy(
+				DeployDecision.Recreate,
+				artifact("built.dex", "gen-1-dex"),
+				null,
+				null,
+				loopStartedAt = 0,
+				recorder = recorder(),
+			)
+			assertThat(store.load()).isNotNull()
 
 			val outcome =
-				deployer().deploy(
+				deployer.deploy(
 					DeployDecision.Restart(ComponentKind.SERVICE, "com.example.app.SyncService"),
-					artifact("built.dex", "dex-bytes"),
+					artifact("built.dex", "gen-2-dex"),
 					null,
 					null,
 					loopStartedAt = 0,
 					recorder = recorder(),
 				)
 
 			assertThat(outcome).isInstanceOf(BuildOutcome.Success::class.java)
-			val retained = store.load()!!
-			// The deploy itself carried restart=true; the re-send must not, or a reconnect
-			// catch-up would ask the just-relaunched app to persist and exit again.
-			assertThat(retained.metadataJson).doesNotContain("restart")
-			assertThat(retained.dexFile!!.readText()).isEqualTo("dex-bytes")
+			// A reconnect catch-up replays retained bytes as a hot swap, which would land on
+			// the live service this deploy restarted for and redefine its classes under it -
+			// the cross-loader CCE the restart existed to prevent. Nothing may stay retained;
+			// a below-deployed reconnect falls back to the forced catch-up rebuild.
+			assertThat(store.load()).isNull()
 		}
 
 	@Test

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

@@ -2,6 +2,7 @@ package org.appdevforall.cotg.quickbuild.service.deploy
 
 import android.os.IBinder
 import android.os.ParcelFileDescriptor
+import android.os.RemoteException
 import com.google.common.truth.Truth.assertThat
 import com.itsaky.androidide.quickbuild.IQuickBuildTarget
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -14,8 +15,9 @@ import org.junit.jupiter.api.Test
 
 /**
  * The uid trust boundary of [QuickBuildHostService.HostBinder], against a real
- * [ProxyAppConnections]. On the JVM the stubbed `Binder.getCallingUid()` reports uid 0,
- * so a session begun for uid 0 stands in for the matching proxy app and any other
+ * [ProxyAppConnections], and the death-watch wiring that keeps the registered target and
+ * the watched binder in step. On the JVM the stubbed `Binder.getCallingUid()` reports
+ * uid 0, so a session begun for uid 0 stands in for the matching proxy app and any other
  * `expectedUid` stands in for a foreign caller.
  */
 @OptIn(ExperimentalCoroutinesApi::class)
@@ -160,6 +162,131 @@ class QuickBuildHostBinderTest {
 		assertThat(connections.target.value).isNull()
 	}
 
+	@Test
+	fun `a binder that is dead at connect is not left registered`() {
+		beginMatchingSession()
+		val dead = WatchableBinder(onLink = { _, _ -> throw RemoteException("already dead") })
+
+		binder.connect(targetOn(dead.binder), "com.example.quickbuild", 0)
+
+		// A dead target no death notification can ever clear would turn every deploy into
+		// a full timeout; the failed link must fail fast to no registration instead.
+		assertThat(connections.target.value).isNull()
+	}
+
+	@Test
+	fun `a dead binder's stale connect retry does not clobber a live registration`() {
+		beginMatchingSession()
+		val live = WatchableBinder()
+		binder.connect(targetOn(live.binder), "com.example.quickbuild", 0)
+		val dead = WatchableBinder(onLink = { _, _ -> throw RemoteException("already dead") })
+
+		binder.connect(targetOn(dead.binder), "com.example.quickbuild", 0)
+
+		// The superseded process's retry lost the race to the fresh process's bind; the
+		// fresh registration must survive it, and stay watched.
+		assertThat(
+			connections.target.value
+				?.target
+				?.asBinder(),
+		).isSameInstanceAs(live.binder)
+		assertThat(live.watching()).hasSize(1)
+	}
+
+	@Test
+	fun `a death delivered while connect is registering still clears the target`() {
+		beginMatchingSession()
+		// linkToDeath delivers the death on another thread immediately, the way a proxy app
+		// crashing right after its connect() call does. The helper returns once that
+		// delivery has either completed or parked against the binder's registration lock,
+		// so both orderings are exercised deterministically rather than raced.
+		var death: Thread? = null
+		val dying =
+			WatchableBinder(
+				onLink = { _, recipient ->
+					val delivery = Thread { recipient.binderDied() }.also { it.start() }
+					death = delivery
+					while (delivery.state != Thread.State.TERMINATED && delivery.state != Thread.State.BLOCKED) {
+						Thread.sleep(1)
+					}
+				},
+			)
+
+		binder.connect(targetOn(dying.binder), "com.example.quickbuild", 0)
+		death!!.join(5_000)
+
+		// Registration and death watch are one atomic step, so the death lands after the
+		// registration and clears it - a dead target must never stay registered.
+		assertThat(connections.target.value).isNull()
+	}
+
+	@Test
+	fun `a reconnect moves the watch, and firing it clears the registration`() {
+		beginMatchingSession()
+		val first = WatchableBinder()
+		val second = WatchableBinder()
+		binder.connect(targetOn(first.binder), "com.example.quickbuild", 0)
+		binder.connect(targetOn(second.binder), "com.example.quickbuild", 1)
+
+		assertThat(first.watching()).isEmpty()
+		val recipient = second.watching().single()
+
+		recipient.binderDied()
+
+		assertThat(connections.target.value).isNull()
+	}
+
+	@Test
+	fun `a graceful disconnect unlinks the death watch`() {
+		beginMatchingSession()
+		val live = WatchableBinder()
+		binder.connect(targetOn(live.binder), "com.example.quickbuild", 0)
+
+		binder.disconnect("com.example.quickbuild")
+
+		// No stale recipient stays linked to fire on the process's eventual death.
+		assertThat(live.watching()).isEmpty()
+	}
+
+	/**
+	 * An [IBinder] that records link/unlink traffic and can script [IBinder.linkToDeath],
+	 * so the death-watch wiring is exercised for real. Only the two death-watch methods
+	 * are live; everything else no-ops through the reflection proxy.
+	 */
+	private class WatchableBinder(
+		private val onLink: (WatchableBinder, IBinder.DeathRecipient) -> Unit = { _, _ -> },
+	) {
+		val linked = mutableListOf<IBinder.DeathRecipient>()
+		val unlinked = mutableListOf<IBinder.DeathRecipient>()
+
+		val binder: IBinder =
+			java.lang.reflect.Proxy.newProxyInstance(
+				IBinder::class.java.classLoader,
+				arrayOf(IBinder::class.java),
+			) { _, method, args ->
+				when (method.name) {
+					"linkToDeath" -> {
+						val recipient = args!![0] as IBinder.DeathRecipient
+						onLink(this, recipient)
+						linked += recipient
+						null
+					}
+
+					"unlinkToDeath" -> {
+						unlinked += args!![0] as IBinder.DeathRecipient
+						true
+					}
+
+					else -> {
+						null
+					}
+				}
+			} as IBinder
+
+		/** The recipients still linked, in link order. */
+		fun watching(): List<IBinder.DeathRecipient> = linked.filterNot { it in unlinked }
+	}
+
 	/**
 	 * A distinct [IBinder] identity. Only reference identity is exercised, so a reflection
 	 * proxy is enough and avoids stubbing the whole interface against an unmocked android.jar.