ADFA-4128: qb-06 review fixes - latching union on failed builds, t3 before retention, broad binder catch, doc corrections

Akash's 08-31 review of #1718, all seven items:

- LiveReloadOrchestrator: the failed-build merge goes through
  unionPendingLocked, so an Unknown batch collapsing over a mid-build
  invalidating edit latches the Gradle verdict instead of erasing it.
  Test pins the collapse (verified red: the manifest edit rode the fast
  daemon path).
- PayloadDeployer: t3 is read before the retention copy (hot swap) and
  the retention clear (restart), keeping post-deploy bookkeeping out of
  the timed save-to-live span. Ordering tests verified red against the
  old order at both sites.
- DeployChannel: the payload send rethrows CancellationException and
  degrades any other exception to DeployResult.Failed, mirroring
  notifyBuildStatus's binder rationale; escape would also dodge the
  not-connected escalation. Test verified red.
- deploy/README: table gains ProxyAppPriorityHold and RetainedPayloadStore.
- ComponentInfo: header rewritten to the declares-one-always-restarts
  rule; supertypes marked carried-but-unread.
- ClassHeader: KDoc aligned with the README's "currently unused".
- E2eTimelineRecorder: false class-header-parse claim dropped.

quickbuild:core tests green (both flavors).

Also: plain-language pass over the comments added by these fixes

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

Author: fryanpan

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/domain/reload/ClassHeader.kt

@@ -4,7 +4,8 @@ import java.io.DataInputStream
 
 /**
  * The hierarchy facts of one compiled class file - name, superclass, directly implemented
- * interfaces - which is what keeps [DeployPolicy]'s supertype index current across builds.
+ * interfaces. Currently unused: [DeployPolicy] keeps no supertype index. Kept for a planned
+ * deploy mode that redefines classes in place and will need the hierarchy.
  *
  * Parsed by a constant-pool walk rather than a bytecode library; these fields sit right after
  * the constant pool, so nothing past the interface list is read. Names are in dot form with

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/domain/reload/ComponentInfo.kt

@@ -3,18 +3,20 @@ package org.appdevforall.cotg.quickbuild.domain.reload
 /**
  * Kind of a manifest component the proxy app build recorded (setup.json `components`).
  *
- * The restart closure referred to throughout this file is [DeployPolicy]'s: a
- * restart-sensitive component class plus its user-side supertypes and their nested classes,
- * any recompile of which forces a proxy-app process restart.
+ * The restart rule referred to throughout this file is [DeployPolicy]'s: a manifest that
+ * declares even one restart-sensitive component makes EVERY deploy a process restart. The
+ * policy does not look at what the compile touched, so there is no per-build set of
+ * affected classes to compute; "outside the restart rule" below means the kind never makes
+ * a deploy restart-sensitive on its own.
  */
 enum class ComponentKind {
-	/** An `<activity>`; outside the restart closure, since recreate already refreshes it. */
+	/** An `<activity>`; outside the restart rule, since recreate already refreshes it. */
 	ACTIVITY,
 
 	/** A `<service>`; a live instance cannot be swapped, so it forces a process restart. */
 	SERVICE,
 
-	/** A `<receiver>`; outside the restart closure, being instantiated fresh per delivery. */
+	/** A `<receiver>`; outside the restart rule, being instantiated fresh per delivery. */
 	RECEIVER,
 
 	/** A `<provider>`; like a service, a live instance forces a process restart. */
@@ -83,7 +85,9 @@ fun ComponentInfo.isRestartSensitive(): Boolean = kind in RESTART_SENSITIVE_KIND
  * @property launcher true for the launcher activity - its [proxyClass] is the explicit
  *   relaunch target after a restart-deploy.
  * @property supertypes the user-side (project-compiled) superclass chain recorded from
- *   class headers at proxy app build time; seeds the restart closure's supertype index.
+ *   class headers at proxy app build time. Carried but currently unread: [DeployPolicy]
+ *   keeps no supertype index. Kept for a planned deploy mode that redefines classes in
+ *   place and will need the hierarchy.
  */
 data class ComponentInfo(
 	val kind: ComponentKind,

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/domain/reload/LiveReloadOrchestrator.kt

@@ -712,7 +712,10 @@ class LiveReloadOrchestrator(
 
 				else -> {
 					val newSavesArrivedMidBuild = !pending.isEmpty || pendingForced
-					pending = flight.batch + pending
+					// unionPendingLocked, not bare plus: if flight.batch is Unknown (built off an
+					// untrusted baseline), plus would collapse the union to Unknown and lose the
+					// full-Gradle-build verdict of a manifest or gradle edit that landed mid-build.
+					pending = unionPendingLocked(flight.batch, pending)
 					// The dead attempt's t0 must not outlive it: its batch is back in pending but
 					// waiting on the user, not queueing, which is not latency this loop owes. A
 					// save that landed MID-build did genuinely queue behind this one, so its

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

@@ -2,6 +2,7 @@ package org.appdevforall.cotg.quickbuild.service.deploy
 
 import android.os.ParcelFileDescriptor
 import android.os.RemoteException
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineStart
 import kotlinx.coroutines.async
 import kotlinx.coroutines.coroutineScope
@@ -193,6 +194,18 @@ class DeployChannel(
 					verdict.cancel()
 					log.error("Deploy of generation {} could not open a payload fd", generation, e)
 					return@coroutineScope DeployResult.Failed("Cannot open payload: ${e.message}")
+				} catch (e: CancellationException) {
+					throw e
+				} catch (e: Exception) {
+					// A binder proxy can throw more than RemoteException (see
+					// notifyBuildStatus below). The throw must come back as a failed
+					// deploy: escaping would crash the deploy loop, and it would bypass
+					// the orchestrator's repeated-failure tally
+					// (LiveReloadOrchestrator.recordFailureLocked), which only sees
+					// failures returned as results.
+					verdict.cancel()
+					log.error("Deploy of generation {} failed past the binder surface", generation, e)
+					return@coroutineScope DeployResult.Failed("Deploy failed: ${e.message}")
 				}
 
 				when (val report = verdict.await()) {

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

@@ -126,13 +126,15 @@ internal class PayloadDeployer(
 			deployRecovering(generation, dexFile, arscFile, assets?.zip, metadata(restart = false))
 		return when (val result = recovered.result) {
 			is DeployResult.Reloaded -> {
-				// The app confirmed the payload, so these bytes are worth retaining for
-				// the reconnect re-send (concurrency.md rules 3-4).
-				retention?.retain(generation, dexFile, arscFile, assets?.zip, metadata(restart = false))
 				// t3: reportReloaded came back from the recreated activity's onResume,
 				// so the new code is live. One clock read feeds both, or the reported
 				// duration would run past the timeline's own total for the same loop.
+				// Read BEFORE the retention copy below: the copy is post-deploy
+				// bookkeeping, not save-to-live latency, so it stays out of the timed span.
 				val liveAt = clock()
+				// The app confirmed the payload, so these bytes are worth retaining for
+				// the reconnect re-send (concurrency.md rules 3-4).
+				retention?.retain(generation, dexFile, arscFile, assets?.zip, metadata(restart = false))
 				reportTimeline(recorder.completed(generation, liveAt))
 				BuildOutcome.Success(generation, liveAt - loopStartedAt)
 			}
@@ -265,11 +267,12 @@ internal class PayloadDeployer(
 				// 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.
+				// launch. One clock read feeds both, as on the hot-swap path - and read
+				// BEFORE the retention drop below, which is bookkeeping outside the span.
 				val liveAt = clock()
+				retention?.clear()
 				reportTimeline(recorder.completed(generation, liveAt))
 				BuildOutcome.Success(generation, liveAt - loopStartedAt, restarted = true)
 			}

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/service/deploy/README.md

@@ -9,3 +9,5 @@ This folder holds the deploy side of the service layer: the exported host servic
 | [`DeployChannel.kt`](DeployChannel.kt) | The on-device `DeploySender`: passes payload files as read-only fds over the oneway `onPayload`, awaits the matching report, and bounds every wait. |
 | [`PayloadDeployer.kt`](PayloadDeployer.kt) | Routes a build's artifacts to hot swap vs process restart, handles relaunch/reconnect and the no-app retry, allocates generations, and maps each `DeployResult` to a `BuildOutcome`. |
 | [`BuildStatusJson.kt`](BuildStatusJson.kt) | Builds the string-valued `statusJson` for `onBuildStatus` (building, build_ok, build_failed, reinstall_pending) the proxy app's overlay reads. |
+| [`ProxyAppPriorityHold.kt`](ProxyAppPriorityHold.kt) | Keeps the connected proxy app out of the cached-app freezer by binding its keep-alive service; `BoundServicePriorityHold` is the on-device implementation. |
+| [`RetainedPayloadStore.kt`](RetainedPayloadStore.kt) | Retains the last confirmed deploy's bytes so a reconnect below the deployed generation can be answered by a re-send instead of a forced rebuild. |

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/service/telemetry/E2eTimelineRecorder.kt

@@ -73,8 +73,7 @@ internal class E2eTimelineRecorder(
 	}
 
 	/**
-	 * Records the hot-swap-versus-restart decision, including the class-header parses it
-	 * needs.
+	 * Records the hot-swap-versus-restart decision.
 	 *
 	 * @param millis host-observed span
 	 */

quickbuild/core/src/test/java/org/appdevforall/cotg/quickbuild/domain/reload/LiveReloadOrchestratorTest.kt

@@ -2202,4 +2202,31 @@ class LiveReloadOrchestratorTest {
 			assertThat(executor.requests.last().route).isEqualTo(BuildRoute.CodeAndResources)
 			assertThat(events.filterIsInstance<OrchestratorEvent.InvalidationRequired>()).isEmpty()
 		}
+
+	@Test
+	fun `a manifest edit landing mid-build survives a failed build collapsing the set to Unknown`() =
+		runTest {
+			// A failed build hands its batch back into pending. When that batch is Unknown
+			// (built off an untrusted baseline) and a manifest edit landed mid-build, a bare
+			// plus collapses the union to Unknown and loses the manifest edit's full-build
+			// verdict - the next save then takes the fast daemon path with a change the
+			// daemon cannot absorb.
+			val executor = GatedExecutor()
+			val events = mutableListOf<OrchestratorEvent>()
+			val orchestrator = LiveReloadOrchestrator(executor, ChangeClassifier(), backgroundScope) { events += it }
+
+			orchestrator.onBaselineUntrusted()
+			orchestrator.onFilesChanged(known(srcA))
+			runCurrent()
+			orchestrator.onFilesChanged(known("app/src/main/AndroidManifest.xml"))
+			runCurrent()
+			executor.finish(0, compileError())
+			runCurrent()
+			orchestrator.onFilesChanged(known(srcB))
+			runCurrent()
+
+			assertThat(executor.requests).hasSize(1)
+			assertThat(events.filterIsInstance<OrchestratorEvent.InvalidationRequired>())
+				.containsExactly(OrchestratorEvent.InvalidationRequired(InvalidationReason.MANIFEST_CHANGED))
+		}
 }

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

@@ -159,6 +159,21 @@ class DeployChannelDeployTest {
 			assertThat((result as DeployResult.Failed).message).contains("Cannot open payload")
 		}
 
+	@Test
+	fun `a payload throw beyond RemoteException degrades to Failed instead of escaping`() =
+		runTest {
+			// A binder proxy can throw more than RemoteException (the reason notifyBuildStatus
+			// catches Exception). An escape here would crash the deploy loop instead of
+			// failing the one deploy, and would bypass the orchestrator's repeated-failure
+			// tally, which only sees failures returned as results.
+			connect(ScriptedTarget(onPayloadThrow = { IllegalStateException("binder went weird") }))
+
+			val result = channel.deploy(3, null, null, null, "{}")
+
+			assertThat(result).isInstanceOf(DeployResult.Failed::class.java)
+			assertThat((result as DeployResult.Failed).message).contains("Deploy failed")
+		}
+
 	@Test
 	fun `a proxy app that never answers times out with the configured timeout`() =
 		runTest {

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

@@ -152,6 +152,92 @@ class PayloadDeployerRetentionTest {
 			assertThat(store.load()).isNull()
 		}
 
+	@Test
+	fun `the retention copy runs outside the timed save-to-live span`() =
+		runTest {
+			// The live-at clock read (t3) happens before retain(): the copy is post-deploy
+			// bookkeeping, and a clock read after it would bill the copy - which scales with
+			// app size, not edit size - to every successful deploy's save-to-live latency.
+			var clockReadAfterRetention = false
+			val deployer =
+				PayloadDeployer(
+					deploy = deploy,
+					generations = GenerationTracker(MemoryGenerationStore()),
+					entryActivity = "com.example.app.MainActivity",
+					proxyAppPackage = "com.example.app",
+					launcherActivity = "com.example.app.Proxy0Activity",
+					launcher = ProxyAppLauncher { _, _ -> true },
+					restartDisconnectTimeoutMillis = 5_000,
+					restartReconnectTimeoutMillis = 15_000,
+					clock = {
+						if (store.load() != null) clockReadAfterRetention = true
+						1_000
+					},
+					reportTimeline = {},
+					retention = store,
+				)
+
+			deployer.deploy(
+				DeployDecision.Recreate,
+				artifact("built.dex", "dex-bytes"),
+				null,
+				null,
+				loopStartedAt = 0,
+				recorder = recorder(),
+			)
+
+			assertThat(store.load()).isNotNull()
+			assertThat(clockReadAfterRetention).isFalse()
+		}
+
+	@Test
+	fun `the restart path's retention clear also runs outside the timed span`() =
+		runTest {
+			// Same ordering rule as the hot-swap path: the clear is bookkeeping, so t3 is
+			// read while the previous deploy's bytes are still retained.
+			var tracking = false
+			var clockReadAfterClear = false
+			val deployer =
+				PayloadDeployer(
+					deploy = deploy,
+					generations = GenerationTracker(MemoryGenerationStore()),
+					entryActivity = "com.example.app.MainActivity",
+					proxyAppPackage = "com.example.app",
+					launcherActivity = "com.example.app.Proxy0Activity",
+					launcher = ProxyAppLauncher { _, _ -> true },
+					restartDisconnectTimeoutMillis = 5_000,
+					restartReconnectTimeoutMillis = 15_000,
+					clock = {
+						if (tracking && store.load() == null) clockReadAfterClear = true
+						1_000
+					},
+					reportTimeline = {},
+					retention = store,
+				)
+			deployer.deploy(
+				DeployDecision.Recreate,
+				artifact("built.dex", "gen-1-dex"),
+				null,
+				null,
+				loopStartedAt = 0,
+				recorder = recorder(),
+			)
+			assertThat(store.load()).isNotNull()
+			tracking = true
+
+			deployer.deploy(
+				DeployDecision.Restart(ComponentKind.SERVICE, "com.example.app.SyncService"),
+				artifact("built.dex", "gen-2-dex"),
+				null,
+				null,
+				loopStartedAt = 0,
+				recorder = recorder(),
+			)
+
+			assertThat(store.load()).isNull()
+			assertThat(clockReadAfterClear).isFalse()
+		}
+
 	@Test
 	fun `a restart deploy whose relaunch never comes back retains nothing`() =
 		runTest {