ADFA-4128: qb 04 review fixes — reload failure attribution + surfacing

- Stale pendingReloadGeneration mis-blaming later crashes: the backgrounded
  apply now assigns the pending slot too (Generations.pendingAfterApply), and
  BootProbation.generationToBlame refuses a pending value the store has moved
  past. Covered by BootProbationTest.aPendingReloadTheStoreMovedPastIsNotBlamed
  (fails without the fix) and GenerationsTest.aBackgroundedApplyClearsThePendingSlotItAlreadyAcked.
- failReload swallowing every pre-apply failure: the newer-generation guard is
  now a three-way Generations.onReloadFailure — never-applied failures skip the
  rollback/quarantine but still reportCrash + banner; only a failure superseded
  by a newer live generation stays silent. Covered by
  GenerationsTest.aFailureTheStoreNeverAdoptedStillReports.
- Binder-thread setProviders + immediate provider close racing main-thread
  inflation: ResourceStore now performs the field swap, setProviders and the
  close of the replaced provider on the main thread (inline when already there,
  so the boot restore path still lands before first inflation; Looper FIFO keeps
  a posted swap ahead of the posted recreate). Pure threading with no JVM seam —
  justified in swapProvidersOnMain's doc; device-covered.

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

Author: fryanpan

quickbuild/runtime/src/main/java/com/itsaky/androidide/quickbuild/runtime/BootProbation.java

@@ -28,13 +28,13 @@ synchronized void bootedFromStore(long generation) {
 	 * The generation a crash happening right now should be quarantined against.
 	 *
 	 * @param pendingReloadGeneration
-	 *            the hot-swapped generation awaiting its first frame, or -1; it outranks the booted one, being the newer claim on the screen that just died
+	 *            the hot-swapped generation awaiting its first frame, or -1; it outranks the booted one, being the newer claim on the screen that just died - unless the store has already moved past it, which means the value is stale (a later deploy acked while backgrounded) and the crash belongs to whatever runs now, not to it
 	 * @param liveGeneration
 	 *            the generation the store currently serves, which is how a booted generation superseded by a later deploy stops being blamed for that deploy's crash
 	 * @return the generation to quarantine, or -1 when nothing this process adopted is to blame
 	 */
 	synchronized long generationToBlame(long pendingReloadGeneration, long liveGeneration) {
-		if (pendingReloadGeneration >= 0) {
+		if (pendingReloadGeneration >= 0 && pendingReloadGeneration >= liveGeneration) {
 			return pendingReloadGeneration;
 		}
 		if (unprovenGeneration >= 0 && unprovenGeneration == liveGeneration) {

quickbuild/runtime/src/main/java/com/itsaky/androidide/quickbuild/runtime/Generations.java

@@ -20,6 +20,41 @@ static boolean accepts(long runningGeneration, long incomingGeneration) {
 		return incomingGeneration > runningGeneration;
 	}
 
+	/**
+	 * What a failed reload owes, decided from where the store stands relative to the failure.
+	 *
+	 * The three cases matter because {@link #rollbackApplies} alone conflates two of them: a failure superseded by a newer deploy must stay silent, but a failure the store never adopted - an oversize payload, a full disk, a restart deploy missing its dex - still has to reach the host and the banner, or its only trace is the host's deploy timeout.
+	 *
+	 * @param runningGeneration
+	 *            generation the store holds right now
+	 * @param failedGeneration
+	 *            generation whose reload failed
+	 * @return the action the failure path must take
+	 */
+	static FailureAction onReloadFailure(long runningGeneration, long failedGeneration) {
+		if (rollbackApplies(runningGeneration, failedGeneration)) {
+			return FailureAction.ROLLBACK_AND_REPORT;
+		}
+		return runningGeneration > failedGeneration
+				? FailureAction.LEAVE_ALONE
+				: FailureAction.REPORT_ONLY;
+	}
+
+	/**
+	 * The pending-reload generation the runtime should hold after a payload applies.
+	 *
+	 * A foreground apply leaves the generation pending until its first resumed frame acks it. A backgrounded apply acks at apply time, and the pending slot must still be assigned - not skipped: leaving an older generation's pending value behind is what let the crash guard blame it for a later generation's crash, and let the crashing generation escape quarantine.
+	 *
+	 * @param resumedActivity
+	 *            whether an activity is resumed, i.e. whether there is a frame to prove the reload on
+	 * @param generation
+	 *            the generation just applied
+	 * @return the value the pending slot must take: the generation while its ack waits for a frame, or -1 when the apply was already acked
+	 */
+	static long pendingAfterApply(boolean resumedActivity, long generation) {
+		return resumedActivity ? generation : -1;
+	}
+
 	/**
 	 * Decides whether a failed reload's rollback still applies.
 	 *
@@ -36,4 +71,14 @@ static boolean rollbackApplies(long runningGeneration, long failedGeneration) {
 	}
 
 	private Generations() {}
+
+	/** What {@link #onReloadFailure} tells the failure path to do. */
+	enum FailureAction {
+		/** The store still holds the failed generation: roll back, quarantine, report, banner. */
+		ROLLBACK_AND_REPORT,
+		/** The store never adopted the failed generation: nothing to roll back or quarantine, but report and banner still fire. */
+		REPORT_ONLY,
+		/** A newer generation owns the store, the pending ack and the screen: touch nothing, say nothing. */
+		LEAVE_ALONE
+	}
 }

quickbuild/runtime/src/main/java/com/itsaky/androidide/quickbuild/runtime/QuickBuildRuntime.java

@@ -17,7 +17,7 @@
  *
  * Installed once per process by {@link QuickBuildAppComponentFactory} at application instantiation; Context work - binding to CoGo, cache dirs - waits for the first activity, since the Application has no base context yet.
  *
- * Failure policy throughout: a reload failure reports the crash and rolls back, so the app keeps running the last working code rather than crash-looping or silently claiming the new generation.
+ * Failure policy throughout: a reload failure reports the crash, and rolls back when the store adopted the failed generation, so the app keeps running the last working code rather than crash-looping or silently claiming the new generation. Only a failure superseded by a newer live generation stays silent.
  */
 final class QuickBuildRuntime {
 
@@ -273,10 +273,13 @@ void handlePayload(long generation, ParcelFileDescriptor dexPayload,
 						PayloadStore.INSTANCE.baselineFingerprint(),
 						application.getCacheDir());
 			}
-			if (tracker.hasResumedActivity()) {
-				pendingReloadStartUptime = startUptime;
-				pendingReloadGeneration = generation;
-			} else {
+			boolean resumed = tracker.hasResumedActivity();
+			pendingReloadStartUptime = startUptime;
+			// Assigned on BOTH branches: the backgrounded ack must also clear any older
+			// generation still pending, or the crash guard keeps blaming it for this
+			// generation's crashes - and this generation escapes quarantine.
+			pendingReloadGeneration = Generations.pendingAfterApply(resumed, generation);
+			if (!resumed) {
 				// Backgrounded: no resumed activity to hang a frame callback on, so
 				// waiting for render-proof would time out a deploy that worked. Ack at
 				// apply+persist, like the restart path.
@@ -487,7 +490,9 @@ private void exitForRestart() {
 	}
 
 	/**
-	 * Rolls back to {@code rollback}, reports the crash to CoGo, and shows the banner; the app stays on the old generation.
+	 * Reports the failure to CoGo and shows the banner; rolls back only when the store adopted the failed generation, so the app stays on the old one either way.
+	 *
+	 * A failure before the apply took - an oversize payload, a persist failure, a restart deploy missing its dex - leaves the store on the previous generation, so there is nothing to restore or quarantine; the report and banner still fire, or the host's only signal would be its deploy timeout. Only a failure superseded by a newer live generation stays silent, since that generation owns the store, the pending ack and the screen.
 	 *
 	 * @param generation
 	 *            the generation that failed, which CoGo marks bad
@@ -497,16 +502,20 @@ private void exitForRestart() {
 	 *            the failure, summarized into both the report and the banner
 	 */
 	private void failReload(long generation, PayloadStore.Payload rollback, Throwable error) {
-		if (!Generations.rollbackApplies(PayloadStore.INSTANCE.generation(), generation)) {
+		Generations.FailureAction action = Generations.onReloadFailure(
+				PayloadStore.INSTANCE.generation(), generation);
+		if (action == Generations.FailureAction.LEAVE_ALONE) {
 			// A newer payload landed while this one was failing, so it owns the store, the
 			// pending ack and the screen. Rolling back here would undo a deploy that worked.
 			RuntimeLog.w("gen " + generation + " failed but gen "
 					+ PayloadStore.INSTANCE.generation() + " is live; leaving it alone", error);
 			return;
 		}
-		PayloadStore.INSTANCE.restore(rollback);
-		quarantine(generation);
-		pendingReloadGeneration = -1;
+		if (action == Generations.FailureAction.ROLLBACK_AND_REPORT) {
+			PayloadStore.INSTANCE.restore(rollback);
+			quarantine(generation);
+			pendingReloadGeneration = -1;
+		}
 		String summary = summarize(error);
 		setOverlayState(OverlayState.crashed(summary));
 		client.reportCrash(generation, summary);

quickbuild/runtime/src/main/java/com/itsaky/androidide/quickbuild/runtime/ResourceStore.java

@@ -6,6 +6,8 @@
 import android.content.res.loader.ResourcesLoader;
 import android.content.res.loader.ResourcesProvider;
 import android.os.Build;
+import android.os.Handler;
+import android.os.Looper;
 import android.os.ParcelFileDescriptor;
 import java.io.File;
 import java.io.IOException;
@@ -209,13 +211,19 @@ private void applyTableLegacy(ParcelFileDescriptor tableFd, long generation,
 	@TargetApi(30)
 	private void applyTableWithLoader(ParcelFileDescriptor tableFd) throws IOException {
 		try {
-			ResourcesProvider next = ResourcesProvider.loadFromApk(tableFd, null);
-			synchronized (this) {
-				ResourcesProvider previous = provider;
-				provider = next;
-				installProviders();
-				Streams.closeQuietly(previous);
-			}
+			final ResourcesProvider next = ResourcesProvider.loadFromApk(tableFd, null);
+			swapProvidersOnMain(new Runnable() {
+
+				@Override
+				public void run() {
+					synchronized (ResourceStore.this) {
+						ResourcesProvider previous = provider;
+						provider = next;
+						installProviders();
+						Streams.closeQuietly(previous);
+					}
+				}
+			});
 		} finally {
 			// loadFromApk dups the fd internally; ours must be closed either way.
 			Streams.closeQuietly(tableFd);
@@ -275,16 +283,54 @@ private void installProviders() {
 	 */
 	@TargetApi(30)
 	private void refreshAssetsProvider(File dir) throws IOException {
-		DirectoryAssetsProvider nextDir = new DirectoryAssetsProvider(dir);
-		ResourcesProvider next = ResourcesProvider.empty(nextDir);
-		synchronized (this) {
-			ResourcesProvider previous = assetsProvider;
-			DirectoryAssetsProvider previousDir = assetsDirProvider;
-			assetsProvider = next;
-			assetsDirProvider = nextDir;
-			installProviders();
-			Streams.closeQuietly(previous);
-			Streams.closeQuietly(previousDir);
+		final DirectoryAssetsProvider nextDir = new DirectoryAssetsProvider(dir);
+		final ResourcesProvider next = ResourcesProvider.empty(nextDir);
+		swapProvidersOnMain(new Runnable() {
+
+			@Override
+			public void run() {
+				synchronized (ResourceStore.this) {
+					ResourcesProvider previous = assetsProvider;
+					DirectoryAssetsProvider previousDir = assetsDirProvider;
+					assetsProvider = next;
+					assetsDirProvider = nextDir;
+					installProviders();
+					Streams.closeQuietly(previous);
+					Streams.closeQuietly(previousDir);
+				}
+			}
+		});
+	}
+
+	/**
+	 * Runs a provider swap on the main thread, inline when already there.
+	 *
+	 * The swap must not run on the binder thread the deploy arrives on: setProviders rebuilds every attached Resources in place and the swap then closes the replaced provider's ApkAssets, either of which can race an inflation already in progress on the main thread - a lookup straddling the swap mixes old and new values, or touches a just-closed provider. Serializing with the main thread removes both races, and Looper FIFO keeps a posted swap ahead of the recreate the deploy posts right after it.
+	 *
+	 * Inline on the main thread, not posted, because the boot restore path runs during the first activity's creation and its swap must land before anything inflates.
+	 *
+	 * A swap failure is logged rather than thrown: on the posted path no caller is left to catch it, and the previous provider set stays live either way, which the next deploy replaces.
+	 *
+	 * @param swap
+	 *            the field swap + setProviders + close of the replaced provider, taking the store's monitor itself
+	 */
+	private void swapProvidersOnMain(final Runnable swap) {
+		Runnable guarded = new Runnable() {
+
+			@Override
+			public void run() {
+				try {
+					swap.run();
+				} catch (Throwable error) {
+					RuntimeLog.e("resource provider swap failed; previous set stays live", error);
+				}
+			}
+		};
+		Looper main = Looper.getMainLooper();
+		if (Looper.myLooper() == main) {
+			guarded.run();
+		} else {
+			new Handler(main).post(guarded);
 		}
 	}
 }

quickbuild/runtime/src/test/java/com/itsaky/androidide/quickbuild/runtime/BootProbationTest.java

@@ -70,6 +70,15 @@ void anUnstampedBaselineIsNotAGenerationWorthRefusing() {
 		assertThat(probation.generationToBlame(NO_PENDING_RELOAD, 0)).isEqualTo(NO_PENDING_RELOAD);
 	}
 
+	@Test
+	void aPendingReloadAheadOfTheStoreIsStillBlamed() {
+		// The failure path's window: gen 11 failed, the store was just restored to 10, and the
+		// pending slot has not been cleared yet. A crash here is still gen 11's doing.
+		BootProbation probation = new BootProbation();
+
+		assertThat(probation.generationToBlame(11, 10)).isEqualTo(11);
+	}
+
 	@Test
 	void aPendingReloadOutranksTheGenerationThisProcessBooted() {
 		// Both are live claims on the screen; the hot swap is the newer one, and it is the one
@@ -80,6 +89,17 @@ void aPendingReloadOutranksTheGenerationThisProcessBooted() {
 		assertThat(probation.generationToBlame(11, 11)).isEqualTo(11);
 	}
 
+	@Test
+	void aPendingReloadTheStoreMovedPastIsNotBlamed() {
+		// Deploy 9 lands foreground and is left pending its first frame; the user backgrounds,
+		// deploy 10 applies and acks in the background. A crash now is gen 10's: blaming stale
+		// 9 would quarantine working code, mislead CoGo, AND let 10 boot again on relaunch -
+		// the exact startup crash-loop the quarantine machinery exists to break.
+		BootProbation probation = new BootProbation();
+
+		assertThat(probation.generationToBlame(9, 10)).isEqualTo(NO_PENDING_RELOAD);
+	}
+
 	@Test
 	void aProcessThatBootedTheInstalledCodeBlamesNothing() {
 		// The baked baseline is the floor a quarantine falls back to. Refusing it would leave

quickbuild/runtime/src/test/java/com/itsaky/androidide/quickbuild/runtime/GenerationsTest.java

@@ -6,15 +6,55 @@
 
 class GenerationsTest {
 
+	@Test
+	void aBackgroundedApplyClearsThePendingSlotItAlreadyAcked() {
+		// The regression: deploy 9 lands foreground and is left pending, the user backgrounds
+		// before its first resumed frame, deploy 10 applies and acks in the background. The
+		// backgrounded apply must still assign the slot - skipping it left stale 9 behind, so
+		// the crash guard blamed 9 for gen 10's crashes and 10 escaped quarantine.
+		assertThat(Generations.pendingAfterApply(false, 10)).isEqualTo(-1);
+	}
+
+	// The stamped-baseline boot gate is pinned in PersistedSelectionTest, against the real
+	// PayloadStore seam - re-numbering accepts() cases here could not fail on that caller.
+
 	@Test
 	void acceptsStrictlyNewerGeneration() {
 		assertThat(Generations.accepts(0, 1)).isTrue();
 		assertThat(Generations.accepts(41, 42)).isTrue();
 		assertThat(Generations.accepts(41, 100)).isTrue();
 	}
 
-	// The stamped-baseline boot gate is pinned in PersistedSelectionTest, against the real
-	// PayloadStore seam - re-numbering accepts() cases here could not fail on that caller.
+	@Test
+	void aFailureSupersededByANewerLiveGenerationStaysSilent() {
+		// Gen 6's posted recreate throws after gen 7 already applied: gen 7 owns the store,
+		// the pending ack and the screen, so gen 6's failure must touch and say nothing.
+		assertThat(Generations.onReloadFailure(7, 6))
+				.isEqualTo(Generations.FailureAction.LEAVE_ALONE);
+	}
+
+	@Test
+	void aFailureTheStoreNeverAdoptedStillReports() {
+		// An oversize payload, a persist failure, a restart deploy missing its dex: the store
+		// still runs the previous generation, so there is nothing to roll back or quarantine -
+		// but the report and banner must fire, or the failure's only trace is the host's
+		// deploy timeout and the developer sees nothing on device.
+		assertThat(Generations.onReloadFailure(5, 6))
+				.isEqualTo(Generations.FailureAction.REPORT_ONLY);
+		assertThat(Generations.onReloadFailure(0, 1))
+				.isEqualTo(Generations.FailureAction.REPORT_ONLY);
+	}
+
+	@Test
+	void aFailureWhileTheFailedGenerationOwnsTheStoreRollsBackAndReports() {
+		assertThat(Generations.onReloadFailure(6, 6))
+				.isEqualTo(Generations.FailureAction.ROLLBACK_AND_REPORT);
+	}
+
+	@Test
+	void aForegroundApplyLeavesItsGenerationPendingItsFirstFrame() {
+		assertThat(Generations.pendingAfterApply(true, 10)).isEqualTo(10);
+	}
 
 	@Test
 	void rejectsEqualGeneration() {