ADFA-4128 (4/11): address CodeRabbit review

- F1716-2 heal a half-finished asset merge on the next run
- F1716-5 stop answering a VirtualMachineError with another allocation
- F1716-7 take the asset length from the descriptor already open
- F1716-8 un-commit a resource provider swap that failed to install

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7

Author: fryanpan

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

@@ -29,6 +29,13 @@ final class AssetExtractor {
 	/** Marker file beside {@link #CURRENT_DIR} naming the baseline the merged assets belong to. */
 	static final String BASELINE_MARKER = "baseline.fp";
 
+	/**
+	 * Marker file beside {@link #CURRENT_DIR} that exists only while a merge is in flight.
+	 *
+	 * Finding it at the start of the next merge means the previous one died part-way, so the merged dir holds two generations. The baseline marker still matches and later payloads carry only newly-changed files, so nothing else would ever heal it - a wrongly-written file would stay wrong until a forced rebuild.
+	 */
+	static final String MERGE_PENDING_MARKER = "merge.pending";
+
 	private static final int BUFFER_SIZE = 16 * 1024;
 
 	/**
@@ -84,6 +91,8 @@ static int extract(InputStream zipStream, File destDir) throws IOException {
 	 *
 	 * The clear-then-mark order is the safe crash window: a death between the two leaves a mismatched marker, so the next call clears an already-empty dir instead of serving another baseline's assets.
 	 *
+	 * A merge that dies part-way is recovered at the START of the next call, not on the failure path: {@link #MERGE_PENDING_MARKER} is written before the first byte and cleared only after the last, and finding it still there clears the dir. A cleared dir is safe - the provider falls through to the APK's baked-in assets - whereas a half-merged one serves a file from the wrong generation.
+	 *
 	 * @param zipStream
 	 *            the changed-assets zip as it arrived over binder; read but never closed
 	 * @param assetsRoot
@@ -101,11 +110,19 @@ static int extractCumulative(InputStream zipStream, File assetsRoot,
 		}
 		File providerRoot = currentDir(assetsRoot);
 		File marker = new File(assetsRoot, BASELINE_MARKER);
-		if (!baselineFingerprint.equals(readMarker(marker))) {
+		File pending = new File(assetsRoot, MERGE_PENDING_MARKER);
+		if (!baselineFingerprint.equals(readMarker(marker)) || pending.isFile()) {
 			deleteRecursively(providerRoot);
 			writeMarker(marker, baselineFingerprint);
 		}
-		return extract(zipStream, new File(providerRoot, ASSETS_SUBDIR));
+		writeMarker(pending, baselineFingerprint);
+		int count = extract(zipStream, new File(providerRoot, ASSETS_SUBDIR));
+		if (!pending.delete()) {
+			// The merge itself is complete and correct, but a marker we cannot clear makes the
+			// next call clear a dir that did not need it. Say so rather than leave it silent.
+			throw new IOException("cannot clear merge marker " + pending);
+		}
+		return count;
 	}
 
 	/**

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

@@ -75,7 +75,10 @@ public AssetFileDescriptor loadAssetFd(String path, int accessMode) {
 		try {
 			ParcelFileDescriptor fd = ParcelFileDescriptor.open(
 					candidate, ParcelFileDescriptor.MODE_READ_ONLY);
-			return new AssetFileDescriptor(fd, 0, candidate.length());
+			// Size from the descriptor, not a second stat of the path: open() pinned an inode,
+			// and an extraction renaming the file in between would otherwise pair the old
+			// inode with the new file's length - a short read, or a read past EOF.
+			return new AssetFileDescriptor(fd, 0, fd.getStatSize());
 		} catch (FileNotFoundException error) {
 			// Raced by a concurrent clear; absent and unreadable look the same to the
 			// framework, which falls through to the baked-in copy.

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

@@ -17,6 +17,20 @@
  */
 public class QuickBuildAppComponentFactory extends AppComponentFactory {
 
+	/**
+	 * Rethrows a throwable the fallback cannot help with, before anything else runs.
+	 *
+	 * A {@link VirtualMachineError} says the VM is out of a resource the retry needs, so logging it (formatting a message, walking a stack trace) and then re-running the same construction allocates again in exactly the state that cannot afford it - and when the default-loader retry happens to succeed, the error is swallowed outright. {@link LinkageError} is deliberately NOT in this set: a stale-payload {@code NoSuchFieldError} is the case the fallback exists for.
+	 *
+	 * @param error
+	 *            what the payload loader threw
+	 */
+	static void rethrowIfFatal(Throwable error) {
+		if (error instanceof VirtualMachineError) {
+			throw (VirtualMachineError) error;
+		}
+	}
+
 	/**
 	 * Throws the failure that best explains a component we could not instantiate from either loader: the PAYLOAD one.
 	 *
@@ -96,6 +110,7 @@ public Activity instantiateActivity(ClassLoader cl, String className, Intent int
 		try {
 			return super.instantiateActivity(pickLoader(cl, className), className, intent);
 		} catch (Throwable payloadError) {
+			rethrowIfFatal(payloadError);
 			RuntimeLog.e("payload activity instantiation failed for " + className
 					+ "; using default loader", payloadError);
 			try {
@@ -129,6 +144,7 @@ public Application instantiateApplication(ClassLoader cl, String className)
 		try {
 			application = super.instantiateApplication(pickLoader(cl, className), className);
 		} catch (Throwable payloadError) {
+			rethrowIfFatal(payloadError);
 			RuntimeLog.e("payload application instantiation failed; using default loader", payloadError);
 			try {
 				application = super.instantiateApplication(cl, className);
@@ -170,6 +186,7 @@ public ContentProvider instantiateProvider(ClassLoader cl, String className)
 		try {
 			return super.instantiateProvider(pickLoader(cl, className), className);
 		} catch (Throwable payloadError) {
+			rethrowIfFatal(payloadError);
 			RuntimeLog.e("payload provider instantiation failed for " + className
 					+ "; using default loader", payloadError);
 			try {
@@ -206,6 +223,7 @@ public BroadcastReceiver instantiateReceiver(ClassLoader cl, String className, I
 		try {
 			return super.instantiateReceiver(pickLoader(cl, className), className, intent);
 		} catch (Throwable payloadError) {
+			rethrowIfFatal(payloadError);
 			RuntimeLog.e("payload receiver instantiation failed for " + className
 					+ "; using default loader", payloadError);
 			try {
@@ -242,6 +260,7 @@ public Service instantiateService(ClassLoader cl, String className, Intent inten
 		try {
 			return super.instantiateService(pickLoader(cl, className), className, intent);
 		} catch (Throwable payloadError) {
+			rethrowIfFatal(payloadError);
 			RuntimeLog.e("payload service instantiation failed for " + className
 					+ "; using default loader", payloadError);
 			try {

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

@@ -219,7 +219,17 @@ public void run() {
 					synchronized (ResourceStore.this) {
 						ResourcesProvider previous = provider;
 						provider = next;
-						installProviders();
+						try {
+							installProviders();
+						} catch (RuntimeException | Error error) {
+							// Un-commit. The loader still holds the previous set, so the field has to as
+							// well - leaving the rejected provider there makes the next deploy offer it
+							// again, and dropping `previous` on the floor leaks a provider that is still
+							// installed. Closing `next` is safe: it was never installed.
+							provider = previous;
+							Streams.closeQuietly(next);
+							throw error;
+						}
 						Streams.closeQuietly(previous);
 					}
 				}
@@ -294,7 +304,17 @@ public void run() {
 					DirectoryAssetsProvider previousDir = assetsDirProvider;
 					assetsProvider = next;
 					assetsDirProvider = nextDir;
-					installProviders();
+					try {
+						installProviders();
+					} catch (RuntimeException | Error error) {
+						// Same un-commit as applyTableWithLoader: restore the fields the loader still
+						// reflects, close the pair that never got installed, and let the failure out.
+						assetsProvider = previous;
+						assetsDirProvider = previousDir;
+						Streams.closeQuietly(next);
+						Streams.closeQuietly(nextDir);
+						throw error;
+					}
 					Streams.closeQuietly(previous);
 					Streams.closeQuietly(previousDir);
 				}
@@ -309,7 +329,7 @@ public void run() {
 	 *
 	 * 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.
+	 * 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. The result is deliberately NOT returned to the deploy chain - that would make a deploy arriving on a binder thread block on a main-thread round trip in the hot reload path, which is the very thing posting the swap exists to avoid. Each swap un-commits its own fields on failure, so what stays live is a consistent previous generation.
 	 *
 	 * @param swap
 	 *            the field swap + setProviders + close of the replaced provider, taking the store's monitor itself

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

@@ -43,6 +43,55 @@ private static InputStream zipOf(Map<String, byte[]> entries) throws IOException
 	@TempDir
 	Path tempDir;
 
+	@Test
+	void aCompletedMergeClearsThePendingMarker() throws IOException {
+		File root = tempDir.resolve("assets-root").toFile();
+		Map<String, byte[]> first = new LinkedHashMap<String, byte[]>();
+		first.put("kept.txt", "from the first merge".getBytes("UTF-8"));
+		AssetExtractor.extractCumulative(zipOf(first), root, "fp-1");
+
+		assertThat(new File(root, AssetExtractor.MERGE_PENDING_MARKER).exists()).isFalse();
+
+		// And because it is clear, the next merge accumulates instead of starting over -
+		// a marker left behind would silently throw the first payload's files away.
+		Map<String, byte[]> second = new LinkedHashMap<String, byte[]>();
+		second.put("added.txt", "from the second merge".getBytes("UTF-8"));
+		AssetExtractor.extractCumulative(zipOf(second), root, "fp-1");
+
+		File assetsDir = new File(AssetExtractor.currentDir(root), AssetExtractor.ASSETS_SUBDIR);
+		assertThat(readFile(new File(assetsDir, "kept.txt"))).isEqualTo("from the first merge");
+		assertThat(readFile(new File(assetsDir, "added.txt"))).isEqualTo("from the second merge");
+	}
+
+	@Test
+	void aMergeThatDiedPartWayIsClearedAtTheStartOfTheNextOne() throws IOException {
+		File root = tempDir.resolve("assets-root").toFile();
+		File assetsDir = new File(AssetExtractor.currentDir(root), AssetExtractor.ASSETS_SUBDIR);
+		Map<String, byte[]> first = new LinkedHashMap<String, byte[]>();
+		first.put("kept.txt", "from the completed merge".getBytes("UTF-8"));
+		AssetExtractor.extractCumulative(zipOf(first), root, "fp-1");
+
+		// One entry lands, the next one escapes the destination and aborts the merge. The
+		// dir now holds two generations and the baseline marker still matches, so nothing
+		// downstream can tell.
+		Map<String, byte[]> partial = new LinkedHashMap<String, byte[]>();
+		partial.put("half.txt", "half-written".getBytes("UTF-8"));
+		partial.put("../evil.txt", "escaped".getBytes("UTF-8"));
+		assertThrows(IOException.class,
+				() -> AssetExtractor.extractCumulative(zipOf(partial), root, "fp-1"));
+		assertThat(new File(assetsDir, "half.txt").exists()).isTrue();
+		assertThat(new File(root, AssetExtractor.MERGE_PENDING_MARKER).isFile()).isTrue();
+
+		Map<String, byte[]> third = new LinkedHashMap<String, byte[]>();
+		third.put("fresh.txt", "after recovery".getBytes("UTF-8"));
+		AssetExtractor.extractCumulative(zipOf(third), root, "fp-1");
+
+		// Same baseline throughout, so only the pending marker could have forced this clear.
+		assertThat(new File(assetsDir, "kept.txt").exists()).isFalse();
+		assertThat(new File(assetsDir, "half.txt").exists()).isFalse();
+		assertThat(readFile(new File(assetsDir, "fresh.txt"))).isEqualTo("after recovery");
+	}
+
 	@Test
 	void cumulativeMergeKeepsEarlierPayloadsFiles() throws IOException {
 		File root = tempDir.resolve("assets-root").toFile();

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

@@ -24,6 +24,13 @@ void aCheckedPayloadFailureKeepsItsOwnType() {
 		assertThat(thrown).isSameInstanceAs(payloadError);
 	}
 
+	@Test
+	void aLinkageErrorIsNotFatal() {
+		// The stale-payload case the default-loader fallback exists for, so it must fall
+		// through to the retry rather than being rethrown here.
+		QuickBuildAppComponentFactory.rethrowIfFatal(new NoSuchFieldError("field removed by a stale payload"));
+	}
+
 	@Test
 	void aRuntimePayloadFailurePropagatesUnwrapped() {
 		NoSuchFieldError payloadError = new NoSuchFieldError("field removed by a stale payload");
@@ -45,6 +52,17 @@ void aThrowableNoSignatureAllowsIsWrappedWithTheCauseIntact() throws Exception {
 		assertThat(wrapper.getCause()).isSameInstanceAs(payloadError);
 	}
 
+	@Test
+	void aVirtualMachineErrorIsRethrownRatherThanRetried() {
+		OutOfMemoryError fatal = new OutOfMemoryError("payload dex would not fit");
+
+		// Retrying the same construction after this would allocate again in exactly the state
+		// that cannot afford it, and a retry that happened to succeed would swallow it entirely.
+		assertThat(assertThrows(
+				OutOfMemoryError.class, () -> QuickBuildAppComponentFactory.rethrowIfFatal(fatal)))
+				.isSameInstanceAs(fatal);
+	}
+
 	@Test
 	void oneThrowableAsBothFailuresDoesNotBlowUpOnSelfSuppression() {
 		RuntimeException error = new RuntimeException("the same instance twice");