ADFA-4128 (4/11): address Akash's review

Fixes for every finding on the runtime module, plus two changes that came out of
reviewing them.

Crash banner. The copy said "New code crashed", which named the one event this
banner cannot observe: the CRASHED state is set only from failReload, so it is
always the reload machinery that failed, never the user's own code. It also
carried a stack summary it had no room for. It now reads

  Live reload crashed. App is on the last working version.
  For more info, see Build Output in Code on the Go.

and points at the pane where the full text already goes unchanged. At the
narrowest width measured on an A56 at 2x font scale that is five rendered lines,
four from 28 characters up; MAX_BANNER_LINES stays at 6, one line of slack,
because the line a tighter cap drops is the tail of the pointer - which leaves
the reader told to look somewhere without the name of the place.

Crash report. It walks up to three causes and prints each one's frames, not just
its toString. An Android lifecycle crash always arrives wrapped, so the top
frames are ActivityThread's every time and the line naming the developer's bug
sits in the cause; reporting the message alone named the exception without ever
placing it.

markGood retry. lastMarkedGoodGeneration was set before the write was attempted,
so a failed markGood was never retried and its latch blocked every later one for
the process lifetime, and the KDoc's justification was inverted. Clearing the
latch on a bare false is not safe either - markGood answers several situations
with one false, and persist runs before apply, so meta.json is briefly ahead of
the live generation on every deploy. markGoodCanSucceed separates a failed write
from a store that moved on, and only the failed write clears.

Payload overtake. onPayload is oneway, so a slower older deploy can be overtaken
while it reads its payload and then publish itself over the newer one, leaving
disk a generation behind the running process until the next cold boot adopts it.
PayloadStore.apply already refuses a generation that is not strictly newer, so
this could never reach the screen - only disk. PayloadPersistence now keeps the
highest generation this process has published and refuses anything older,
throwing StalePayloadException so the deploy path can tell a lost race from a
broken store and stay silent about it. The bar rises only after the publishing
rename, so a persist that threw part-way does not block its own retry.

That guard also separates the two cases a generation number alone conflates. A
restarted host counter - the project's state dir wiped while the app stays
installed - always arrives in a process that has published nothing, so the mark
is zero and the low generation is adopted as before. Both counter-restart tests
now build a fresh store object over the same directory, which is the only shape
that case has on a device.

Payload memory. The resource apk and the assets zip were read whole into memory
and written straight back out to files that are reopened as files afterwards, so
a cold deploy held two payload-sized arrays live for no benefit on the devices
least able to spare them. persist now takes both as streams and copies them
through a 16 KB buffer into the same temp-then-fsync-then-rename write. Only the
dex stays a byte array, because InMemoryDexClassLoader needs one. The 64 MB cap
is unchanged and now guards the streaming path; the parameter types are what
keep it that way.

Also from Akash: the manifest-merger comment, the KDoc corrections, and the test
helper that divided length by width - it modelled a renderer that breaks
mid-word, so it read the banner's six real lines as five and could not have
caught the overflow it existed for. It wraps on words now, and was watched
failing at the old cap before the cap moved.

Both new gates were watched red first: the payload cap with its check stubbed
out, the overtake refusal with its condition forced false. Only the intended
test failed each time. 248 tests green.

Banner inset. Photographing the new banner at 2x font scale showed it drawing
over the status bar: getRootWindowInsets() comes back null on the first render
after a config-change recreate, and the overlay took that as a 0 inset. A null
read now means "not measured yet" - the margin is left alone and re-read after
the next layout, once, by a listener that removes itself. The decision is a pure
static so it can be unit-tested; the deferred re-read firing is checked on a
device (A56: banner flush below the 101 px bar at 1.0 and 2.0).

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

Author: fryanpan

quickbuild/runtime/build.gradle.kts

@@ -37,9 +37,9 @@ android {
 // map parsing, asset extraction). Mirrors :quick-build's jupiter setup.
 tasks.withType<Test> {
 	useJUnitPlatform()
-	// StreamsTest exercises the 256 MB payload cap through the default readFully
-	// overload; a capped reader legitimately buffers up to the cap before throwing,
-	// which overflows Gradle's default 512 MB test-worker heap.
+	// StreamsTest exercises the payload cap through the default readFully overload; a
+	// capped reader legitimately buffers up to the cap and then copies it, so the peak is
+	// about twice the cap - more headroom than Gradle's default 512 MB test-worker heap.
 	maxHeapSize = "1g"
 }
 

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

@@ -0,0 +1,95 @@
+package com.itsaky.androidide.quickbuild.runtime;
+
+/**
+ * Turns a payload crash into the text CoGo is told about it, and owns the height its banner may reach.
+ *
+ * One summary, one reader. {@link #forReport} crosses binder to CoGo, which has a screen, a scrollback and the developer's attention, so it carries enough frames to place the fault. The banner over the user's own app carries none of it - it names Build Output and stops - so what remains here for the banner is {@link #MAX_BANNER_LINES} alone.
+ */
+final class CrashSummary {
+
+	/**
+	 * Lines {@link StatusOverlay}'s banner shows before it ellipsizes, sized against the whole rendered banner.
+	 *
+	 * The crash banner is the tallest state whose text this class controls: a headline of 56 characters and {@code OverlayState.FULL_OUTPUT_POINTER} at 50. Wrapped at 25 characters per line - the narrowest width measured on an A56 at 2x font scale - the headline costs 3 rendered lines and the pointer 2, so the banner is 5 at its tallest.
+	 *
+	 * Five is needed across the 25-to-27 band; from 28 characters up the headline folds into two as well and the banner fits in 4. The cap is left one line above the worst measured case rather than tightened onto it, because the line it would drop is the tail of the pointer - the reader is left told to look somewhere, without the name of the place - and because a font or locale wider than anything measured here should cost a blank line, not a truncated instruction.
+	 *
+	 * An earlier version also put a stack summary on the banner and needed 14 lines to fit it. Dropping the summary is what buys this back, so putting any detail on the banner again means recomputing here rather than raising the cap. {@code StatusOverlay} reads this instead of carrying a number of its own that could drift from it.
+	 *
+	 * One state is outside this arithmetic: {@code BUILD_FAILED}'s detail is a diagnostic line CoGo sends and nothing here caps its length, so a long one still ellipsizes. That predates this budget and is not addressed by it.
+	 */
+	static final int MAX_BANNER_LINES = 6;
+
+	/** Frames a report to CoGo names; enough to place the fault, short enough to read. */
+	private static final int MAX_REPORT_FRAMES = 5;
+
+	/** Hard cap on the report form, since it crosses binder. */
+	private static final int MAX_REPORT_LENGTH = 2000;
+
+	/**
+	 * Causes the report walks past the top throwable.
+	 *
+	 * An Android lifecycle crash always arrives wrapped - the framework rethrows as "Unable to start activity" - so the top throwable's frames are ActivityThread's and the frame naming the developer's bug is one level down. Reporting the top alone spends the whole budget on frames no reader can act on. Three levels covers a wrapped cause and the two rewraps a build pipeline tends to add; {@link #truncate} is the backstop, and it cuts from the deepest cause, which is the end worth losing.
+	 */
+	private static final int MAX_REPORT_CAUSES = 3;
+
+	/**
+	 * The full form reported to CoGo.
+	 *
+	 * @param error
+	 *            the failure to summarize; must be non-null
+	 * @return the exception and up to {@link #MAX_REPORT_CAUSES} causes, each with up to {@link #MAX_REPORT_FRAMES} frames, truncated to {@link #MAX_REPORT_LENGTH} chars
+	 */
+	static String forReport(Throwable error) {
+		StringBuilder sb = new StringBuilder();
+		sb.append(error.toString());
+		appendFrames(sb, error, MAX_REPORT_FRAMES);
+		// Each cause gets its frames too, not just its toString. The frame a developer needs is
+		// almost never in the top throwable: the framework wraps a lifecycle crash, so the top
+		// five frames are ActivityThread's every time and the one line naming the bug sits in
+		// the cause. Reporting the message alone named the exception without ever placing it.
+		Throwable cause = error.getCause();
+		Throwable previous = error;
+		for (int depth = 0; cause != null && cause != previous && depth < MAX_REPORT_CAUSES; depth++) {
+			sb.append("\nCaused by: ").append(cause.toString());
+			appendFrames(sb, cause, MAX_REPORT_FRAMES);
+			previous = cause;
+			cause = cause.getCause();
+		}
+		return truncate(sb, MAX_REPORT_LENGTH);
+	}
+
+	/**
+	 * Appends at most {@code limit} of {@code error}'s frames, one per line.
+	 *
+	 * @param sb
+	 *            the summary under construction
+	 * @param error
+	 *            the failure whose trace to read; a trace stripped by the VM is simply empty
+	 * @param limit
+	 *            the most frames to append
+	 */
+	private static void appendFrames(StringBuilder sb, Throwable error, int limit) {
+		StackTraceElement[] frames = error.getStackTrace();
+		int count = Math.min(frames.length, limit);
+		for (int i = 0; i < count; i++) {
+			sb.append("\n at ").append(frames[i]);
+		}
+	}
+
+	/**
+	 * @param sb
+	 *            the summary under construction
+	 * @param limit
+	 *            the most characters to keep
+	 * @return the summary, cut to {@code limit} characters
+	 */
+	private static String truncate(StringBuilder sb, int limit) {
+		if (sb.length() > limit) {
+			sb.setLength(limit);
+		}
+		return sb.toString();
+	}
+
+	private CrashSummary() {}
+}

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

@@ -19,33 +19,26 @@
 @TargetApi(30)
 final class DirectoryAssetsProvider implements AssetsProvider, Closeable {
 
+	private final File root;
+
 	/**
-	 * Whether {@code candidate} resolves strictly inside {@code root}.
-	 *
-	 * Both sides are canonicalized, so {@code ..} segments and symlinks resolve before the comparison rather than being compared as text. The trailing separator is what stops a sibling whose name merely starts with the root's - {@code /a/rootEvil} against root {@code /a/root} - and it also excludes {@code root} itself.
-	 *
-	 * @param root
-	 *            the override directory being served
-	 * @param candidate
-	 *            a path resolved against it
-	 * @return true when the candidate may be served; false when it escapes, or when either path cannot be canonicalized - unresolvable counts as outside, since a path this process cannot resolve is one it must not serve
+	 * The root's canonical path with a trailing separator, resolved once: {@code root} is final, so its canonical form cannot change over the provider's lifetime. Null when the root could not be canonicalized, which refuses every lookup.
 	 */
-	static boolean isWithinRoot(File root, File candidate) {
-		try {
-			return candidate.getCanonicalPath().startsWith(root.getCanonicalPath() + File.separator);
-		} catch (IOException error) {
-			return false;
-		}
-	}
-
-	private final File root;
+	private final String rootPrefix;
 
 	/**
 	 * @param root
 	 *            the directory to serve, laid out as an APK root (asset files under {@code assets/})
 	 */
 	DirectoryAssetsProvider(File root) {
 		this.root = root;
+		String prefix;
+		try {
+			prefix = root.getCanonicalPath() + File.separator;
+		} catch (IOException unresolvableRoot) {
+			prefix = null;
+		}
+		this.rootPrefix = prefix;
 	}
 
 	/** Nothing held open between lookups; here so {@link ResourceStore} can treat providers uniformly. */
@@ -66,7 +59,7 @@ public AssetFileDescriptor loadAssetFd(String path, int accessMode) {
 		File candidate = new File(root, path);
 		// Same containment rule as AssetExtractor: the path arrives from outside
 		// this process's control and must not resolve outside the override dir.
-		if (!isWithinRoot(root, candidate)) {
+		if (!isWithinRoot(candidate)) {
 			return null;
 		}
 		if (!candidate.isFile()) {
@@ -85,4 +78,24 @@ public AssetFileDescriptor loadAssetFd(String path, int accessMode) {
 			return null;
 		}
 	}
+
+	/**
+	 * Whether {@code candidate} resolves strictly inside the served root.
+	 *
+	 * Both sides are canonicalized, so {@code ..} segments and symlinks resolve before the comparison rather than being compared as text. The root's half is resolved in the constructor instead of here, because this runs for every asset the app opens and this provider sits ahead of the baked-in APK in the loader's list. The trailing separator is what stops a sibling whose name merely starts with the root's - {@code /a/rootEvil} against root {@code /a/root} - and it also excludes the root itself.
+	 *
+	 * @param candidate
+	 *            a path resolved against the root
+	 * @return true when the candidate may be served; false when it escapes, or when either path could not be canonicalized - unresolvable counts as outside, since a path this process cannot resolve is one it must not serve
+	 */
+	boolean isWithinRoot(File candidate) {
+		if (rootPrefix == null) {
+			return false;
+		}
+		try {
+			return candidate.getCanonicalPath().startsWith(rootPrefix);
+		} catch (IOException error) {
+			return false;
+		}
+	}
 }

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

@@ -7,6 +7,13 @@
  */
 final class OverlayState {
 
+	/**
+	 * Where a reader goes for the whole failure, named on the crash banner.
+	 *
+	 * The banner is a strip over the user's own app and deliberately cannot scroll, so it names the surface that holds the failure rather than carrying any of it. Build Output is the pane CoGo already writes the reported summary to, so this points at something that exists rather than something we would have to build. Its length is half of {@link CrashSummary#MAX_BANNER_LINES}' arithmetic - lengthening it without revisiting that clips the pointer itself, which is the one line the banner cannot afford to lose.
+	 */
+	static final String FULL_OUTPUT_POINTER = "For more info, see Build Output in Code on the Go.";
+
 	/**
 	 * State for a compile error, carrying the message summary the banner names. The banner is position-free by design: the error location is CoGo's to show, so it never crosses the deploy channel.
 	 *
@@ -30,14 +37,14 @@ static OverlayState building(long runningGeneration) {
 	}
 
 	/**
-	 * State for a payload that crashed and was rolled back, with a stack summary as {@code detail}.
+	 * State for a payload that crashed and was rolled back.
+	 *
+	 * The banner takes no stack summary. It covers the user's own app, so it says what happened and names where the detail is, and stops; {@link CrashSummary#forReport} still ships the whole thing to CoGo. A summary here would be text nobody can scroll, on a surface that ellipsizes, in front of an app the reader did not ask us to cover.
 	 *
-	 * @param detail
-	 *            one-line summary of the crash, appended to the banner; null renders the headline alone
 	 * @return the crash state
 	 */
-	static OverlayState crashed(String detail) {
-		return new OverlayState(Kind.CRASHED, detail, 0, -1);
+	static OverlayState crashed() {
+		return new OverlayState(Kind.CRASHED, null, 0, -1);
 	}
 
 	/**
@@ -125,8 +132,11 @@ String text() {
 			}
 			return sb.toString();
 		case CRASHED:
-			return "New code crashed - app is running the last working version"
-					+ (detail == null ? "" : "\n" + detail);
+			// "Live reload crashed", not "new code crashed": this state is set only from
+			// failReload, so it is the reload machinery that failed, never the user's own
+			// code. The old wording named the one event this banner cannot observe.
+			return "Live reload crashed. App is on the last working version." + "\n"
+					+ FULL_OUTPUT_POINTER;
 		case REINSTALL_PENDING:
 			return "Update needs your OK in Code on the Go - switch back to approve it\n"
 					+ "This app is running the last working version";