ADFA-4128: qb 09 review fixes — snapshot defense, baseline semantics, d8 + stable-ids surfacing

Review findings (PR #1721, all four Important items):

1. Stale shrunk-snapshot on re-configure -> configure fingerprints the classpath
   jars (path+size+CRC) and wipes shrunk-classpath-snapshot.bin plus ic/ when the
   bytes changed, keeping them when identical. Covered by IncrementalCompilerTest
   "re-configuring over an in-place rewritten classpath jar discards the stale
   shrunk snapshot" and its byte-identical keep-warm companion.

2. "Deployed" baseline that no deploy ever acks -> deployedOutputs renamed to
   lastGoodOutputs with honest KDoc, and a compile declaring EVERY source changed
   now rebaselines: the output diff runs against nothing and reports the whole
   tree, giving clients a wire-compatible recovery after a failed dex/deploy.
   Covered by IncrementalCompilerTest "declaring every source changed rebaselines
   - the whole output tree is reported changed". ROUTED(qb-08 core-orchestration
   / qb-11 app): the orchestrator must still force a full-changed compile
   (ChangedFiles.Unknown) after a failed dex/deploy; today it only re-queues the
   batch. No protocol-module change.

3. d8 diagnostics not captured -> a DiagnosticsHandler proxy is installed via
   D8Command.builder(handler); collected error diagnostics are appended (bounded)
   to the Failed message instead of the bare "Compilation failed to complete".
   Covered by DexToolEdgeTest "a d8 failure surfaces d8's own error diagnostics,
   not only the generic message" (runtime-compiled fake r8, runs untethered).

4. Silent stable-ids degrade -> relink fails a named-but-missing stableIds file
   before aapt2 runs; only an explicit null links unpinned. Covered by
   Aapt2LinkEdgeTest "a named but missing stable-ids file fails the relink
   instead of silently linking unpinned".

Tests are written to fail without their fix but were NOT executed here (no-build
constraint on this fix pass); verify with :quickbuild:daemon:test.

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

Author: fryanpan

quickbuild/daemon/src/main/kotlin/org/appdevforall/cotg/quickbuild/daemon/DaemonService.kt

@@ -179,7 +179,9 @@ class DaemonService(
 	 * Compiles the requested sources and reports the changed class outputs plus phase timings.
 	 *
 	 * @param request must list every module source in `allSources`, not only the edited ones,
-	 *   and repeat them all in `changedFiles` on a session's first compile.
+	 *   and repeat them all in `changedFiles` on a session's first compile - and again to
+	 *   rebaseline after a failed dex or deploy, which makes `classesChanged` report the whole
+	 *   tree instead of a diff against outputs the device never received.
 	 * @return ok with `classesDir`, the phase timings and the `classesChanged` path list, or
 	 *   ok:false carrying the compiler diagnostics; ok:false if no `configure` ran first.
 	 */
@@ -278,6 +280,8 @@ class DaemonService(
 	 *
 	 * @param request `stableIds` and `libraryResources` are optional on the wire but omitting
 	 *   either risks a wrong-id crash or an unresolvable reference - see [Aapt2Link]'s KDoc.
+	 *   A `stableIds` path that is supplied but missing on disk fails the relink rather than
+	 *   silently linking unpinned.
 	 * @return ok with `resourcesArsc` (the full relinked apk) and the aapt2 timings, or ok:false
 	 *   carrying the aapt2 diagnostics; ok:false if no `configure` ran first.
 	 */

quickbuild/daemon/src/main/kotlin/org/appdevforall/cotg/quickbuild/daemon/compile/IncrementalCompiler.kt

@@ -31,6 +31,11 @@ private typealias OutputSnapshot = Map<String, Pair<Long, Long>>
  * - The caller must pass ALL sources as changed on the first compile, to seed the IC caches.
  * - `assureNoClasspathSnapshotsChanges(true)` is only safe once the shrunk snapshot exists;
  *   before that the engine needs the full classpath comparison to seed.
+ * - A shrunk snapshot left in [workDir] by a previous session describes THAT session's
+ *   classpath bytes, so `init` fingerprints the jars and discards the snapshot plus the IC
+ *   caches on a mismatch - otherwise the first compile asserts "classpath unchanged" over a
+ *   classpath a standard Gradle build may have rewritten in place, and stale dependents ship
+ *   silently (see [discardStaleIncrementalState]).
  *
  * Java sources take two passes: kotlinc reads them for symbol resolution only, then javac
  * compiles them after Kotlin into the same output dir, which is what compiles Kotlin<->Java
@@ -66,8 +71,11 @@ class IncrementalCompiler(
 		 * @property warnings kotlinc's and javac's warnings, already parsed into the protocol
 		 *   shape; a successful compile can still carry them.
 		 * @property changedClassFiles the .class files this compile emitted, rewrote or deleted,
-		 *   relative to [classesDir]; the deploy policy picks restart vs recreate from it, so it
-		 *   is diffed against the last DEPLOYED state and includes deletions.
+		 *   relative to [classesDir], for the deploy policy to read. Diffed against the LAST
+		 *   SUCCESSFUL COMPILE's tree - no deploy ack reaches the daemon, so that tree is only a
+		 *   proxy for what the device runs. A compile that declares every source changed is a
+		 *   rebaseline and reports the whole tree; a client whose dex or deploy failed recovers
+		 *   "changed vs installed" accuracy exactly that way (see [compile]).
 		 * @property kotlinMillis wall time of the Kotlin pass (0 when there are no Kotlin sources).
 		 * @property javaMillis wall time of the javac pass (0 when there are no Java sources).
 		 * @property stats the phases [kotlinMillis]/[javaMillis] do not cover - the two
@@ -138,16 +146,20 @@ class IncrementalCompiler(
 	private var pendingJavaAbi: Map<File, JavaSourceAbi.FileAbi>? = null
 
 	/**
-	 * The output tree as of the last compile the caller could deploy; null before the first one.
-	 * Held across compiles for the same reason [javaAbi] is: a failed compile leaves output nobody
+	 * The output tree of the last SUCCESSFUL compile; null before the first one. Not the last
+	 * DEPLOYED tree: no deploy ack reaches the daemon, so a compile whose dex or deploy fails
+	 * still promotes here, and the client must rebaseline (declare every source changed, or
+	 * re-configure) before the diff means "changed vs installed" again. Held across FAILED
+	 * compiles for the same reason [javaAbi] is: a failed compile leaves output nobody
 	 * deployed, so re-snapshotting at the top of the next compile would adopt those undeployed
 	 * classes as already-live and drop them from [Result.Success.changedClassFiles].
 	 */
-	private var deployedOutputs: OutputSnapshot? = null
+	private var lastGoodOutputs: OutputSnapshot? = null
 
 	init {
 		Files.createDirectories(icCachesDir)
 		Files.createDirectories(classesDir)
+		discardStaleIncrementalState(classpathJars)
 		val snapshotDir = workDir.resolve("cp-snap")
 		Files.createDirectories(snapshotDir)
 		// Snapshot the fixed session classpath once; a classpath change is a session
@@ -165,12 +177,66 @@ class IncrementalCompiler(
 			}
 	}
 
+	/**
+	 * Discards a previous session's shrunk snapshot and IC caches when this session's classpath
+	 * BYTES differ from the ones that produced them.
+	 *
+	 * The shrunk snapshot is keyed by path alone (`<rootProjectDir>/shrunk-classpath-snapshot.bin`),
+	 * so it survives a re-configure into the same [workDir] - and [compileKotlin] then runs
+	 * `assureNoClasspathSnapshotsChanges(true)` over a classpath a standard Gradle build may have
+	 * rewritten in place (same jar paths, new ABI). Any compile trusting that assertion keeps
+	 * dependents of the changed library ABI stale, the worst silent failure this feature has.
+	 * Fingerprinting path+size+CRC of every jar catches the in-place rewrite; a mismatch (or a
+	 * missing fingerprint next to surviving state) wipes both, and the next compile re-seeds from
+	 * the fresh per-jar snapshots. Matching bytes keep the warm caches, which re-configures with
+	 * an unchanged classpath must not lose.
+	 *
+	 * @param classpathJars the session classpath, fingerprinted before the per-jar snapshots are
+	 *   computed over it.
+	 */
+	private fun discardStaleIncrementalState(classpathJars: List<File>) {
+		val fingerprintFile = workDir.resolve("classpath-fingerprint.txt").toFile()
+		val fingerprint =
+			classpathJars.joinToString("\n") { jar ->
+				"${jar.absolutePath}|${jar.length()}|${if (jar.isFile) contentCrc(jar) else -1L}"
+			}
+		val previous = if (fingerprintFile.isFile) fingerprintFile.readText() else null
+		if (previous != fingerprint) {
+			shrunkSnapshot.delete()
+			icCachesDir.toFile().deleteRecursively()
+			Files.createDirectories(icCachesDir)
+		}
+		fingerprintFile.writeText(fingerprint)
+	}
+
+	/**
+	 * CRC32 of a whole file, streamed - classpath jars run tens of MB, so no
+	 * [checksumOf]-style whole-file read on a 2-4 GB phone.
+	 *
+	 * @param file the jar to checksum; must exist.
+	 * @return the CRC32 of its bytes.
+	 */
+	private fun contentCrc(file: File): Long {
+		val crc = CRC32()
+		file.inputStream().use { input ->
+			val buffer = ByteArray(FINGERPRINT_READ_BUFFER_BYTES)
+			while (true) {
+				val read = input.read(buffer)
+				if (read < 0) break
+				crc.update(buffer, 0, read)
+			}
+		}
+		return crc.value
+	}
+
 	/**
 	 * Runs one compile: the incremental Kotlin pass, then javac over any `.java` sources.
 	 *
 	 * @param allSources every source in the module, not just the edited ones.
 	 * @param changedFiles sources edited since the last compile; pass all of [allSources] on
-	 *   the first compile of a session.
+	 *   the first compile of a session. Declaring every source changed is also the REBASELINE
+	 *   signal: the output diff then runs against nothing, reporting the whole tree as changed,
+	 *   which is how a client recovers after a failed dex or deploy (see [lastGoodOutputs]).
 	 * @param removedFiles sources deleted since the last compile, no longer in [allSources];
 	 *   their stale `.class` outputs are cleaned before anything is compiled.
 	 * @return [Result.Failed] on any compile error, and also when a removed source's stale
@@ -201,8 +267,14 @@ class IncrementalCompiler(
 		compileCount++
 		javaAbiSnapMillis = 0
 		kotlinToCompileCount = 0
+		// A compile declaring EVERY source changed is a rebaseline: the session's first compile,
+		// or a client that stopped trusting what the device runs because a dex or deploy failed
+		// (no deploy ack reaches the daemon - see lastGoodOutputs). Diffing it against the last
+		// compile's never-deployed tree would answer "nothing changed" for classes the device
+		// has never received, so it diffs against nothing and reports the whole tree.
+		val rebaseline = allSources.isNotEmpty() && changedFiles.toSet().containsAll(allSources)
 		val preSnapStartedAt = System.currentTimeMillis()
-		val before = deployedOutputs ?: snapshotClassOutputs()
+		val before = if (rebaseline) emptyMap() else (lastGoodOutputs ?: snapshotClassOutputs())
 		val preSnapMillis = System.currentTimeMillis() - preSnapStartedAt
 		val logger = CollectingLogger(compileLog)
 		val kotlinStartedAt = System.currentTimeMillis()
@@ -267,8 +339,9 @@ class IncrementalCompiler(
 		val changedClassFiles = changedClassOutputs(before, after)
 		val postSnapMillis = System.currentTimeMillis() - postSnapStartedAt
 		// Same rule as the ABI above: this output only becomes the baseline because the caller
-		// can now deploy it.
-		deployedOutputs = after
+		// can now deploy it - whether the deploy then LANDS is invisible here, which is why a
+		// client whose deploy failed must rebaseline (see the field).
+		lastGoodOutputs = after
 		return Result.Success(
 			classesDir = classesDir.toFile(),
 			warnings = warnings + javaDiagnostics.diagnostics,
@@ -332,7 +405,7 @@ class IncrementalCompiler(
 	/**
 	 * Diffs two output-tree walks into the paths the deploy has to account for.
 	 *
-	 * @param before the last deployed state.
+	 * @param before the last successful compile's state, or empty on a rebaseline.
 	 * @param after this compile's state.
 	 * @return added, rewritten AND deleted paths - a deletion has to be in here, since dropping a
 	 *   nested class of a restart-sensitive component is a change the deploy policy must see and
@@ -611,5 +684,8 @@ class IncrementalCompiler(
 	companion object {
 		// ART (via d8 desugaring) handles Java-17 bytecode; matches the bundled JDK.
 		private const val JVM_TARGET = "17"
+
+		/** Read-chunk size for [contentCrc]'s streamed jar checksum. */
+		private const val FINGERPRINT_READ_BUFFER_BYTES = 64 * 1024
 	}
 }

quickbuild/daemon/src/main/kotlin/org/appdevforall/cotg/quickbuild/daemon/dex/DexTool.kt

@@ -2,7 +2,10 @@ package org.appdevforall.cotg.quickbuild.daemon.dex
 
 import org.appdevforall.cotg.quickbuild.protocol.DexStats
 import java.io.File
+import java.lang.reflect.InvocationHandler
 import java.lang.reflect.InvocationTargetException
+import java.lang.reflect.Method
+import java.lang.reflect.Proxy
 import java.net.URLClassLoader
 import java.nio.file.Files
 import java.nio.file.Path
@@ -85,9 +88,10 @@ class DexTool(
 		if (classFiles.isEmpty()) {
 			return Result.Failed("no .class files found under: ${classesDirs.joinToString()}")
 		}
+		val diagnostics = D8DiagnosticsCollector()
 		return try {
 			val d8StartedAt = System.currentTimeMillis()
-			runD8(classFiles, outDir.toPath())
+			runD8(classFiles, outDir.toPath(), diagnostics)
 			val d8Millis = System.currentTimeMillis() - d8StartedAt
 			val dexFiles = dexFilesIn(outDir)
 			val failure = dexFailureReason(dexFiles, outDir)
@@ -102,29 +106,54 @@ class DexTool(
 				)
 			}
 		} catch (e: InvocationTargetException) {
-			Result.Failed("d8 failed: ${e.cause?.message ?: e.cause?.javaClass?.name ?: e.message}")
+			Result.Failed(d8FailureMessage(e, diagnostics.errors))
 		} catch (e: ReflectiveOperationException) {
 			Result.Failed("d8 jar is not usable (wrong build-tools layout?): ${e.message}")
 		}
 	}
 
+	/**
+	 * The caller-facing message for a d8 compilation failure. The exception's own message is
+	 * near-useless (typically just "Compilation failed to complete"); the real reasons -
+	 * duplicate class, unsupported class file version, malformed input - arrive as error
+	 * diagnostics on the [D8DiagnosticsCollector], so they are appended, bounded so one
+	 * pathological run cannot flood the response.
+	 *
+	 * @param e the reflective d8 failure; its cause's message leads.
+	 * @param errors the run's collected error diagnostics, possibly empty.
+	 * @return one message carrying the cause and every collected error, newline-separated.
+	 */
+	private fun d8FailureMessage(
+		e: InvocationTargetException,
+		errors: List<String>,
+	): String {
+		val cause = "d8 failed: ${e.cause?.message ?: e.cause?.javaClass?.name ?: e.message}"
+		if (errors.isEmpty()) return cause
+		return cause + "\n" + errors.joinToString("\n").take(MAX_DIAGNOSTIC_CHARS)
+	}
+
 	/**
 	 * Builds and runs a D8 command reflectively against the device's r8 jar.
 	 *
 	 * @param classFiles the already-stripped `.class` copies, passed as d8 program inputs.
 	 * @param outDir d8's output dir, written in `DexIndexed` mode.
+	 * @param diagnostics receives the run's diagnostics; without it d8 prints its real failure
+	 *   reasons to the default handler's stderr, which the client only ever logs.
 	 * @throws java.lang.reflect.InvocationTargetException wrapping any d8 compilation error.
 	 * @throws ReflectiveOperationException when the r8 jar does not expose the expected API.
 	 */
 	private fun runD8(
 		classFiles: List<Path>,
 		outDir: Path,
+		diagnostics: D8DiagnosticsCollector,
 	) {
 		val commandClass = loader.loadClass("com.android.tools.r8.D8Command")
 		val outputModeClass = loader.loadClass("com.android.tools.r8.OutputMode")
+		val handlerClass = loader.loadClass("com.android.tools.r8.DiagnosticsHandler")
 		val dexIndexed = outputModeClass.enumConstants.first { (it as Enum<*>).name == "DexIndexed" }
 
-		val builder = commandClass.getMethod("builder").invoke(null)
+		val handler = Proxy.newProxyInstance(loader, arrayOf(handlerClass), diagnostics)
+		val builder = commandClass.getMethod("builder", handlerClass).invoke(null, handler)
 		val builderClass = builder.javaClass
 		builderClass
 			.getMethod("addProgramFiles", Collection::class.java)
@@ -194,7 +223,92 @@ class DexTool(
 		loader.close()
 	}
 
+	/**
+	 * Stands in for r8's `DiagnosticsHandler` behind a [Proxy], collecting the error messages
+	 * of one d8 run - so a failed dex can surface WHY (duplicate class, unsupported class file
+	 * version, malformed input) instead of the exception's generic "Compilation failed to
+	 * complete". Reflective throughout: it may reference no r8 type, since r8 loads through
+	 * [DexTool]'s private class loader.
+	 *
+	 * `internal` so the reflective message extraction and the pass-through arms are
+	 * unit-testable against fake handler/diagnostic interfaces - real d8 needs a host
+	 * toolchain.
+	 */
+	internal class D8DiagnosticsCollector : InvocationHandler {
+		/** Messages of the run's error diagnostics, in report order. */
+		val errors = mutableListOf<String>()
+
+		override fun invoke(
+			proxy: Any,
+			method: Method,
+			args: Array<out Any>?,
+		): Any? {
+			val argument = args?.firstOrNull()
+			return when (method.name) {
+				"error" -> {
+					if (argument != null) errors += diagnosticMessage(method, argument)
+					null
+				}
+
+				// Keep whatever level d8 proposed - returning null here would NPE inside d8.
+				"modifyDiagnosticsLevel" -> {
+					argument
+				}
+
+				// Object's methods reach the handler too on a Proxy.
+				"hashCode" -> {
+					System.identityHashCode(proxy)
+				}
+
+				"equals" -> {
+					proxy === argument
+				}
+
+				"toString" -> {
+					"D8DiagnosticsCollector"
+				}
+
+				// warning/info, and anything the interface grows later: droppable for a failure
+				// report. Void methods take null; others echo a compatible argument so a future
+				// pass-through default keeps working.
+				else -> {
+					if (method.returnType == Void.TYPE) null else args?.firstOrNull { method.returnType.isInstance(it) }
+				}
+			}
+		}
+
+		/**
+		 * Reads `getDiagnosticMessage()` - and, best-effort, the origin - through the PUBLIC
+		 * r8 `Diagnostic` interface, which is the handler method's parameter type. Never through
+		 * the argument's own class: d8's diagnostic implementations are typically
+		 * package-private, and invoking a public method through a non-public class throws
+		 * `IllegalAccessException`.
+		 *
+		 * @param method the intercepted handler method, whose parameter type is the interface.
+		 * @param diagnostic the reported diagnostic object.
+		 * @return the diagnostic's message, origin-prefixed when one is available.
+		 */
+		private fun diagnosticMessage(
+			method: Method,
+			diagnostic: Any,
+		): String {
+			val diagnosticType = method.parameterTypes.firstOrNull() ?: return diagnostic.toString()
+			val message =
+				runCatching {
+					diagnosticType.getMethod("getDiagnosticMessage").invoke(diagnostic) as? String
+				}.getOrNull() ?: diagnostic.toString()
+			val origin =
+				runCatching {
+					diagnosticType.getMethod("getOrigin").invoke(diagnostic)?.toString()
+				}.getOrNull()
+			return if (origin.isNullOrBlank() || origin == "unknown") message else "$origin: $message"
+		}
+	}
+
 	companion object {
+		/** Cap on the collected-diagnostics tail of a d8 failure message. */
+		private const val MAX_DIAGNOSTIC_CHARS = 4000
+
 		/** `classes.dex`, `classes2.dex`, ... - d8's DexIndexed output names, and nothing else. */
 		private val DEX_FILE_NAME = Regex("""classes\d*\.dex""")