ADFA-4128: Quick Build - on-device live reload - #1669
Conversation
59396fa to
768b8b1
Compare
|
What does "every core port" mean? |
|
In the first table, the row "app wiring" says "The only unit touching existing code — the IDE integration: the toolbar button, the Koin module binding every core port to Android, provisioning, the Gradle heap strategies, AndroidManifest.xml." What does "core port" mean? Is it about networking? |
|
Good catch — that was jargon, and not even the useful kind. It's nothing to do with networking. I've reworded the row to say what it actually means:
The reason it's built that way: core never names an Android class, so its logic is unit-testable on the JVM. The |
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Code review: Quick Build (ADFA-4128)
Reviewed the full diff (~40k lines of new production code, 414 files) across seven parallel passes - core domain, core data+deploy, core session+provision, daemon+protocol, runtime AAR, gradle-plugin, app wiring - plus every modified pre-existing file. 23 findings are posted inline below, each verified against the branch.
The three that break users outright:
LiveSession.kt:118-SwitchableExecutorsilently swallowsmarkCurrentBuildUserInitiated(), so a bolt tap during an in-flight save-build never relaunches a closed proxy app.ActivityTracker.java:49- the first activity after any cold start never receives the resource loader: persisted-generation dex against the baseline resource table, so stale strings/layouts andNotFoundExceptionfor new IDs.PayloadStore.java:112- sibling payload classloaders mean(MyApp) getApplication()throwsClassCastExceptionafter an activity-only edit.
Also worth blocking on: MIN_API is decoded by the daemon but never sent by the host, so every hot payload dexes at API 30 while the baseline uses the project's minSdk - that contradicts this PR's API 28/29 claim.
Merge hazard, not a bug. app/src/main/AndroidManifest.xml is reformatted in full (518 lines) with zero content change versus the merge base, and the branch is 10 commits behind stage - including stage's own manifest changes (ExternalFileInstallActivity, the configChanges axes from ADFA-4934). That guarantees a whole-file conflict where a careless resolution silently drops those. Drop the reformat or rebase before merge.
One thing to ack deliberately. composite-builds/.../MavenPublishConf.kt goes from an empty stub back to a full implementation (needed for the gradle-plugin functional tests). It looks safe - no workflow runs a publish task and all three consuming modules have descriptions - but it is a real re-enablement of Maven Central publishing config, worth an explicit nod rather than riding along in this PR.
editor/.../ContentReadWrite.kt is pure Spotless reformatting, no behavior change.
| internal class SwitchableExecutor( | ||
| @Volatile var delegate: LiveReloadExecutor, | ||
| ) : LiveReloadExecutor { | ||
| override suspend fun execute(request: BuildRequest): BuildOutcome = delegate.execute(request) |
There was a problem hiding this comment.
SwitchableExecutor swallows markCurrentBuildUserInitiated()
SwitchableExecutor overrides only execute. LiveReloadExecutor.markCurrentBuildUserInitiated() has a = Unit default, so the call is absorbed here and never reaches the delegate.
Every live session wraps the real executor in this class (LiveSessionFactory.kt:79), so a tap landing on an in-flight save-triggered build is reported as "handled" but LiveReloadExecutorImpl.currentBuildUserInitiated stays false. PayloadDeployer then refuses to relaunch a closed proxy app and the tap produces only a deploy-failure banner - exactly the case the interface KDoc describes.
Forward the override to the delegate.
There was a problem hiding this comment.
Confirmed and fixed in b9d40b6. The override forwards to the delegate now, with a test covering both sides of a baseline swap.
| } | ||
| ClassLoader loader = dex == null | ||
| ? previous.classLoader | ||
| : new InMemoryDexClassLoader(dex, apkClassLoader); |
There was a problem hiding this comment.
Sibling payload classloaders produce ClassCastException across a swap
Each code deploy builds new InMemoryDexClassLoader(dex, apkClassLoader) as a sibling of the previous payload loader, and the payload dex carries all user classes - so the same user class has a distinct identity per generation.
DeployPolicy only restarts the process when the changed set reaches the Application's closure. After an activity-only edit, the recreated activity resolves MyApp through loader N while getApplication() still returns a loader N-1 instance, so (MyApp) getApplication() throws ClassCastException. Same for any live service or user object held across the swap.
There was a problem hiding this comment.
Confirmed — and reproduced on device: a one-line activity edit crashes the app with this cast, twice (the retained payload re-sends and crashes again). Fixed on this branch in b9d40b6: a code deploy now restarts whenever the app declares its own restart-sensitive component, with a cooperative relaunch that puts the user back on the same screen and back stack (~0.4 s vs 13 ms for a hot swap). CoGo's injected logsender components are exempt by exact FQN — they ship only in the base APK dex, so no payload ever redefines them — which keeps hot swap for apps that declare nothing themselves; 5 of 29 corpus apps are affected. The rule, the exemption's safety argument, and the measured costs are documented in quickbuild/docs/live-reload-alternatives.md on this branch. Four follow-on fixes came out of the same device runs: a relaunch that Android swallowed is retried; quarantining a bad generation falls back to the last one that reached the screen instead of to install-time code; a crash on a restart-booted generation now quarantines it too; and the restart handoff waits until the system server has been told the saved state, closing a race that lost the screen and back stack on foreground saves. In-place redefinition via JVMTI (what Apply Changes uses) is drafted as a followup ticket. Decision record: quickbuild/docs/live-reload-alternatives.md on this branch.
| * watcher, and deploy invisibly behind an Idle UI, and the next tap would overwrite | ||
| * [live] leaving that watcher orphaned. Cancelling [sessionWork] from inside it is safe. | ||
| */ | ||
| private fun teardown() { |
There was a problem hiding this comment.
teardown() never cancels an in-flight proxy-app build
teardown() cancels sessionWork but never calls provisioner.cancelProxyAppBuild; only reduceProvisioning's CancelRequested pairs the two, and SessionReducer.kt:35 emits TeardownAndProvision on its own.
So "Restart session" during the ~90s proxy-app build (long-press menu, or the won't-stay-up dialog) leaves Gradle running, and the immediate re-provision hits isBuildInProgress -> SlotBusy -> a generic "Quick Build setup failed". The user's explicit restart fails.
There was a problem hiding this comment.
Confirmed and fixed in b9d40b6. Cancelling the coroutine only abandoned the await — Gradle kept running and holding the build slot, so the restart's own reprovision came back SlotBusy. teardown() now cancels the in-flight build, guarded so the stop tap does not cancel twice; two tests.
| return null | ||
| } | ||
|
|
||
| c == '\\' && i + 1 < n -> { |
There was a problem hiding this comment.
Escaped newline desyncs code and mask, dropping the watcher batch
An escaped newline inside a string literal appends to code but MASKED to mask, desyncing the two line counts. scan then indexes bodyMask out of bounds on any source containing \n inside a literal, and the watcher batch is silently dropped.
There was a problem hiding this comment.
Confirmed and fixed in b9d40b6 — prepare() returns null when the line counts disagree, with tests for the string- and char-literal cases. Worse than a dropped batch: on this path the throw was uncaught and took CoGo down.
| return | ||
| } | ||
| // A pane that never comes back (the user left the editor) must not grow this forever. | ||
| if (pending.size >= MAX_PENDING) { |
There was a problem hiding this comment.
Narrator and flash state outlive their project and activity
Narrator pending is a process singleton never cleared on project close, so project A's lines flush into project B's Build Output pane after a switch.
Related: QuickBuildFlashes.kt:49 keeps flashedFailure as an activity field against a process-lifetime status flow, so an activity recreation (a font-scale change, per CLAUDE.md's 2x check) re-raises an indefinite error flashbar the user already dismissed.
There was a problem hiding this comment.
Confirmed on both halves and fixed in b9d40b6. The flash history lives on the EditorViewModel now, so recreation cannot reset the repeat guard, and the narrator resets its queue on project close so one project's lines cannot flush into the next project's Build Output.
| onFirstAcquire() | ||
| notifyHeldChanged(true) | ||
| } | ||
| try { |
There was a problem hiding this comment.
onFirstAcquire() runs outside the try, so a throw strands the bracket
onFirstAcquire() runs after getAndIncrement() but outside the try, contradicting the KDoc's "nothing runs between the acquire and the try". A throw there leaves the counter incremented with no matching release: the bracket is held forever and every later build sees the slot busy.
There was a problem hiding this comment.
Confirmed and fixed in b9d40b6. The increment is the last statement before the try now, and a failed acquire releases — the safe direction for a UI hint. Two tests, both watched go red with the old ordering restored.
The pipeline in eight steps, the design contract each step honors, and the decisions that are easy to get wrong on a second read: why the daemon is a separate process, why generations only move forward, why a save must not steal the screen. - quickbuild/README.md is the entry point: terms, the eight-step pipeline, benchmark results, how to test, and the decision log. - pipeline.md walks each step in depth (it absorbed the old architecture.md). - component-proxying-design.md explains the proxy-app pattern and its limits. - resource-updates.md and concurrency.md cover the two areas where the design is least obvious from the code. - debugging.md is the triage runbook, reliability-gaps.md the honest list of known gaps with file:line citations, manual-qa.md the block-by-block device plan. - incremental-javac-design.md, ksp-kapt-feasibility.md, low-spec-devices.md and why-not-android-jar.md record the roads not taken and why. - ADR 0012 records the decision to compile outside Gradle, named to the pass and the build its numbers came from. ADR 0002 is scoped to match, since it no longer describes the only build path. Read this first. Everything below it is new modules with no callers until the app-wiring commit, so the branch is meant to be read bottom-up. Two later edits fold in here, since every quickbuild/docs page lives in this commit: debugging.md documents the compileOrdinal field on the e2e log line, and resource-updates.md records why parking a generateSources request is not enough on its own to stop it being dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE Claude-Session: https://claude.ai/code/session_01EMAzZxcPSpxqbYdTxHaZhx
Nothing here is Quick Build itself; it is the host-side surface the feature needs and a few fixes found on the way. - FeatureFlags gates Quick Build so it can ship dark. It tracks whether the sentinel files were actually readable rather than inferring absence from an all-false read, so a direct-boot read cannot latch the feature off. - ToolsManager stages the daemon jar and runtime AAR out of assets. - BuildService gains the hand-back hook a live session needs when an ordinary Gradle build rewrites build/ underneath it. - FlashbarActivityUtils grows a keyed debouncing action so a burst of notices cannot stack banners; Flashbar and ContentReadWrite get the small changes that supports. SaveResult carries the flags the tap's save ordering needs. - The Quick Build toolbar iconography (bolt, building, stop, error) and its strings; TooltipTag gains the Quick Build help entry. - settings.gradle.kts registers the four quickbuild modules; version catalog, publishing and module config entries for them. ARCHITECTURE.md's module map gains quickbuild. analyze.yml sets REQUIRE_BUILD_TOOLCHAIN=1 so the daemon's toolchain tests cannot skip green on a runner with no SDK. Also fixes four unrelated tests that were failing or flaky when this branch started: LogUtils, corrupt-jar classpath reading, Termux shell-manager NPE, and the debouncing-action cancel case. ProjectManagerImpl.generateSources now reports whether it actually dispatched. It early-returns silently when a Gradle build is already in progress, so a caller that owes the request a retry previously had no way to tell a dispatch from a refusal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE Claude-Session: https://claude.ai/code/session_01EMAzZxcPSpxqbYdTxHaZhx
A tiny standalone module so the IDE side and the daemon side cannot disagree about the wire. Requests, results, diagnostics, and a codec, with a malformed input taxonomy that distinguishes wrong types from wrong values - the daemon reads whatever a broken client sends, so parsing has to fail precisely rather than throw. The types live in their own package rather than the module root, so a reader can tell wire format from transport at a glance and neither side can widen the contract by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE
The session state machine and everything it drives: watching for edits, classifying them, deciding a build route, running it, and deploying the result. Laid out package-per-concern (domain/ split from service/), each package with a README stating its contract. - One build in flight; a newer edit supersedes an older one; generations only move forward. - Change classification decides the cheapest route that is still correct - annotation-aware, because a Room @query edit cannot take the fast path. - Trailing-debounce coalescing so a save burst is one build, with late echo batches absorbed rather than stranded. - Deploy policy chooses reload versus component restart; the deployer owns relaunch, reconnect, and the retry when no app is connected. A forced no-op deploy still ships every asset, and a retained payload is re-sent rather than rebuilt. - Daemon lifecycle with an epoch protocol, so a respawn cannot be adopted by the session that asked for the previous one, and a replaced daemon cannot report its own death as the live one's. - A tap carries one bit: it supersedes the build it replaces and is consumed, so it neither forces a blind rebuild nor gets swallowed by a parked session. Correctness is what picks the route, not speed, and one case where the two disagree is worth naming here because it looks like a missing optimization. The proxy app runtime serves deployed assets through a ResourcesLoader AssetsProvider, which exists only on API 30+. Below that the runtime still extracts an asset payload but nothing reads it, so an asset edit acked "reloaded" would leave the app on stale assets - a silent never-stale violation. ChangeClassifier therefore takes assetsLiveReloadable, and when it is false any changed set carrying an asset routes to a full Gradle build. The gate is on the flag rather than on the AssetsOnly arm because changed assets ride in every route's deploy payload, so a code+asset edit would otherwise still ship assets nothing serves. Resources are deliberately untouched: 28/29 have their own LegacyResourceSwap path. The value is threaded down from the Android edge (see the app-wiring commit) rather than read here, keeping SDK routing out of the pure classes. The route reports UNSUPPORTED_FILE_CHANGED, whose meaning already covers a watched file the live reload path cannot deliver. Timing is charged to the save that earned it, not to a dead attempt's, and a save that arrives while the compiler is down is answered and narrated rather than dropped. The e2e log line carries compileOrdinal. It had five stamps and no ordinal, so a 3.0 s build and a 0.9 s build looked like variance rather than two ends of a warm-up curve, and the ordinal was only recoverable by pulling the bench-events feed afterwards. The five stamps still lead and do not move, so the harness's unanchored parser keeps matching and historical runs parse unchanged. The field is omitted rather than printed as 0 when no compile ran: a resources-only relink genuinely has no ordinal, and a 0 there would read as the coldest possible build - the precise misreading the field exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE Claude-Session: https://claude.ai/code/session_01EMAzZxcPSpxqbYdTxHaZhx
A long-lived JVM that keeps kotlinc's incremental caches warm between edits. This is where the speedup comes from: a cold compiler per edit is most of what makes an ordinary on-device build slow. - Incremental Kotlin and Java compilation, with the ABI fingerprint that decides whether a .java edit forces a Kotlin recompile. The fingerprint hashes the file's imports, since an import change is an ABI change downstream. - d8 dexing, aapt2 resource linking, and toolchain discovery over the messy SDK layouts real devices actually have. The aapt2 subprocess is bounded by a timeout that kills it, so a hung linker cannot wedge a session. - final-stripping so a user class can be subclassed by a generated proxy. - A changed-class set the deploy policy can trust, and a split payload that fails rather than deploying half of itself. - An exception backstop on every op: the daemon exits on shutdown, EOF, or a fatal internal error, and never on a handler throwing. A compiler Error is answered as a failed build rather than taken as fatal. - A session releases its tools only once its replacement exists, so a restart cannot leave a window with no compiler. - An offline guard that fails the build if any production class in the module references a network API. The aapt2/d8/Compose regression tests (ADFA-4128 bugs 5/6/8) are assumption-guarded through TestSdk, so on a runner without an Android SDK they would skip green and take that coverage with them. The analyze workflow sets REQUIRE_BUILD_TOOLCHAIN=1 to turn an absent toolchain into a hard failure instead. The runner does have an SDK - Assemble V8 Debug above could not run otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE
Quick Build cannot swap a class the manifest names directly, so the plugin generates Proxy<N><Type> subclasses and rewrites the manifest to name those instead. The manifest then points at something stable while the code behind it changes. - Decides which components can be proxied at all, and why each rejection happens. - Walks a user class up to its framework supertype to pick the right proxy shape. - Synthesizes an activity-alias under each real activity class name so in-app navigation by explicit class keeps resolving. - Emits quickbuild.json, the contract the device side reads back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE
The half that lives inside the proxy app. Receives a payload over a binder channel and swaps code, resources, and assets into the running process. - Classloader routing so freshly compiled classes win over the ones the app started with. - Resource swap with three strategies by API level: ResourcesLoader on 30+, a reflective shim on 28/29, unsupported below. - Payload persistence that survives the crash window between writing bytes and recording the generation they belong to. A deploy is all-or-nothing on disk, and a payload that fails to apply is quarantined rather than left to be read back as good. - The app boots at a monotonic stamped baseline generation, so a restart cannot resurrect a payload older than the build the app was made from. - Reload confirmation is render-proof when an activity is resumed; when the app is backgrounded the runtime acks at apply time instead, so a plain save never times out and never brings the app forward. - A keep-alive service holds the proxy app out of Android's cached-app freezer, so a deploy to a backgrounded app is not silently stalled by the platform. - A build-failure overlay while the app is foreground; a hand-rolled JSON parser rather than a dependency, since this AAR ships inside the user's app. The parser reports its own failure rather than the fallback's. - Service-connect hardening: a RuntimeException while binding is caught like a dead binder, and the reconnect backoff only resets after a successful connect. Assets ride the same loader as the resource table. They shipped end-to-end but were never served: the overlay was built with a null AssetsProvider and the only accessor (overrideAsset) had no callers, so modified assets silently served stale content and new assets crashed on read. The extracted assets now go through a DirectoryAssetsProvider (open-coded; the framework's is not public API) over one cumulative override dir, wrapped in ResourcesProvider.empty for assets-only payloads and installed alongside the table provider on API 30+. Payloads carry only changed assets, so extraction merges into the cumulative dir instead of per-generation dirs, keyed to the baseline fingerprint and cleared on mismatch - the same trigger that discards a persisted payload - so assets never outlive their baseline. Two limits documented rather than fixed. The overlay can add and replace but cannot hide, so a deleted asset stays readable until the next proxy app rebuild. And below API 30 there is no loader to hang the provider on at all, which is why the classifier routes asset-bearing edits to a full Gradle build there instead of deploying assets nothing would read - never stale, at full-build cost. The README limitations row and pipeline.md say both. Two comments corrected against measurement on an A56 (Android 16), because both asserted a timing property the code does not have. The recreate is not deferred to the next resume - the tracker still holds a stopped-but-not-destroyed activity, so the relaunch is scheduled immediately; the ack-at-apply is right for a different reason, which is that there is no resumed activity to hang a frame callback on. And onResume is not a rendered frame: it precedes the first draw, so a resumed-path timing understates time-to-pixels by roughly 4 ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE Claude-Session: https://claude.ai/code/session_01EMAzZxcPSpxqbYdTxHaZhx
The IDE-side integration: a Quick Build action next to Run, session lifecycle tied to the editor, provisioning the proxy app on first use, and narration of every stage where the user already looks. - QuickBuildManager owns the session; provisioning runs the proxy-app build through the normal Gradle path and reports progress, and a stop tap cancels it rather than leaving it orphaned. - Build stages narrate into the Build Output pane and the bottom status bar, same as an ordinary Gradle build; failures show BUILD FAILED there, and the next landed build overwrites it. A delivery failure is narrated as a delivery failure, not a build failure, and a parked rebaseline as the failure it is. - Stale standard-build error banners no longer replay on lifecycle re-collect (the error state is consumed after first display). - The status bar reports "live reloaded in X ms" rather than a generation number - generations are internal bookkeeping; developers only care that the reload landed and how fast (Bryan, QA walk). - A build-type switch is confirmed when the app id is unknown, and the switch is held until the rebaseline it depends on lands. - The save-time generateSources is deferred until Quick Build goes idle and fires only for resource XML, and the eager prebuild is staggered off the project-open spike, so neither competes with the edit the user just made. - The internal-build bracket is released on every exit path. - Kaspresso e2e coverage: pipeline and smoke tests, a flag-off test, plus automation helpers. Narration survives backgrounding. Narrating from a collector inside the editor activity's repeatOnLifecycle(STARTED) loses builds twice over: a build the user backgrounded CoGo to watch narrated into a cancelled collector, and the status StateFlow's replay on return arrived as a first emission, which quickBuildOutputLines rightly says nothing about. Manual QA saw the newest generation's timing and nothing before it. The two collectors are split: the status bar stays lifecycle-scoped - it shows state, not history, so a cancelled collector costs it nothing - while narration moves to QuickBuildOutputNarrator, attached to the session manager's status for as long as the session exists. The editor activity now only binds the pane, and lines produced while none is bound (backgrounded, or between two activities) queue in the narrator until one is. Each landed build's stage timings also route into the pane. They ride the metrics port rather than the session status - E2eTimeline is the only type carrying the per-stage split - so QuickBuildOutputMetricsSink forwards them and quickBuildTimingLine renders the stages that actually ran: "Quick Build: generation 5 - compiled in 2.8s, dexed in 0.4s, relinked in 2.3s (total 6.0s)." The Koin module is also where the classifier's assetsLiveReloadable flag is read - Build.VERSION.SDK_INT >= R, evaluated once at the Android edge and threaded down, so nothing in quickbuild:core has to know about SDK levels. The Quick Build help row is a documentation.db hand-off rather than a host-side write: the app declares the tooltip tag and the invocation site, and the row itself is the documentation asset's to ship. GenerateSourcesDeferral clears a parked request only once the build has actually been dispatched. Parking it while a Quick Build session is live is not enough on its own: generateSources refuses silently whenever any Gradle build holds the single slot, including one this class cannot see from session state - a project sync, or the user's own Run, while the session reads as settled. The release then fired into a refusal and the request was cleared with nothing left to retry it, so a resource save landing in that window left the Java LSP's R symbols stale until the next save. A refused request now stays parked and retries on the same grace window, bounded so a durable refusal gives up rather than burning timers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE Claude-Session: https://claude.ai/code/session_01EMAzZxcPSpxqbYdTxHaZhx
…at they say The debug-only benchmark surface: an adb-triggerable trampoline, the event and metrics recorders behind it, and the comparator fix that decides what the published speedup is a speedup over. The standard-build number stopped early. `standard_build_finished` fires on BuildState.AwaitingInstall, so every published comparison had counted a Gradle build with no APK install and no app launch, against a Quick Build number that includes its full deploy and reload. The comparison was biased against Quick Build; the interesting part is that nothing said so. MODE_STANDARD_E2E runs the same Run action and keeps measuring: it installs, launches, and stamps `standard_install_started`, `standard_install_finished` and `standard_e2e_finished` (carrying tapToRunningMs from the tap instant). The span starts where MODE_STANDARD's does - the timestamp is taken on the line before `runQuickBuild`, which is the call the toolbar Run action makes - so the two modes share a start and differ only in where they stop. A separate mode rather than a flag, because folding install into MODE_STANDARD would silently redefine `standard_build_finished`, and a number whose meaning changed under a name that did not is how two passes get compared as if they measured the same thing. Two dialogs sit in that path and neither can be answered by an unattended device, so a bench run takes the no-dialog route: the Quick-Build-clobber confirmation is bypassed (the session is restarted directly, which is what confirming does), and the launch skips `launchAppAfterInstall` and its prompt by launching from an override of `onInstallationResult` instead of reaching super. The human path is untouched - both bypasses are gated on the e2e latch. The system installer's own confirmation is not ours to suppress; it needs the REQUEST_INSTALL_PACKAGES appop pre-granted, which the harness already does. The span ends when startActivity returns, which is the last instant this process can observe; first frame is a further wait only the framework sees, so the number UNDERSTATES tap-to-usable. Said in the code so nobody reads the field as more than it is. Failure paths stamp too - a build that never reaches an installable APK, an install that reports no package, and a failed launch each end the span with a reason, so a missing measurement is a labelled terminal state rather than an absent row, and the latch is released, which is what stops the next run's collector attributing a human's install to the bench. The whole surface is debug-only and cannot be reached in a release build: the trampoline activity is declared in a debug-source-set manifest, and QuickBuildBenchHooks has an inert release twin so the main sources can call it unconditionally. The activity is exported by necessity - adb shell holds no START_ANY_ACTIVITY, so a non-exported activity could not be driven at all - and is therefore gated on android.permission.DUMP, which adb shell holds, root bypasses, and no third-party app can obtain. Without that gate the feature flags were the only protection, and those are files in the public Downloads directory that any app with storage access can create. The MODE_STANDARD_E2E path is host-compiled only, not yet verified on a device. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTUVfxid5riL2T78piiKDE
…art rule Everything since the reviewed head, as one commit. Three bodies of work: 1. All 26 findings from John's PR #1669 review addressed. Every mechanical defect fixed with tests (notably: C1 activity-attach ordering, C7 phantom deletions, C14 final-class cache, C16 stranded saves, C20 backslash-literal crash); the four product questions decided by Bryan and implemented: C5 notices queue for delivery on return; C6 test-source saves are ignored with a once-per-session notice; C15 the Quick Build action greys out while a standard build runs; C17 the clobber warning moved to tap time, and install re-checks against the applicationId parsed from the APK itself. 2. The C3 fix: a code deploy restarts the app whenever it declares a restart-sensitive component (custom Application, Service, or Provider), because a loader swap cannot update a held instance and the cross-generation ClassCastException follows. With it, the machinery that makes the restart liveable: a cooperative relaunch that restores screen, saved state, and back stack; a handoff that waits for every activity to stop and then drains the main looper, so the saved state demonstrably reaches system_server before the process dies; a retry for a relaunch Android swallowed; and crash safety - a crash on a hot-swapped OR restart-booted generation quarantines it (BootProbation), the next boot falls back to the last generation that reached the screen (good.json), and quarantine refuses the good generation so the fallback cannot loop. 3. The scope exemption: the components CoGo itself injects (logsender's service and provider, matched by exact FQN) do not trigger the restart rule, in both consumers - DeployPolicy and the stale-helpers notice. They ship only in the base APK dex, so no payload ever redefines them; without the exemption every app restarts on every save (logsender is injected into every debuggable build), and the measured cost of that is +477 ms median per save (A56, 2026-08-21, 42 paired saves, 3 apps). quickbuild/docs/live-reload-alternatives.md now documents the shipped method (rule, exemption and its safety argument, relaunch, crash safety, costs), with the alternatives evaluation kept as history. pipeline.md and component-proxying-design.md updated to match. Tests: quickbuild:core 1093, app 534, 0 failures; exemption, probation, handoff, clobber, test-source and grey-out changes each mutation-gated. Device-verified on a Samsung A56 (two passes, 2026-08-20/21): foreground saves keep screen and back stack, quarantine falls back and refuses the good generation, C3 repro stays fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013rHdmCyZY3MF6CJK7dS2U3
80760f0 to
b9d40b6
Compare
|
Thanks for this review — it was thorough and it held up: all 23 inline findings were verified against the branch and all 23 were real, zero false positives. Every one is fixed, and each inline comment now carries its reply. What happened to the branch since you read it. The head you reviewed ( The manifest merge hazard: good catch. The whitespace-only reformat (it was exactly CRLF→LF normalization, 259 lines) is gone — the rebase onto
Verification on the pushed head: |
3 participants
Quick Build lets the user run a live reload loop that updates their already running test app in seconds -- instead of going through an incremental Gradle build and reinstall. This opens as a draft so the team can start playing with it now; the remaining known fixes land on this PR as batched commits rather than followup PRs.
1. Overview
Please read the
quickbuild/README.mdfirst -- it's a good overview map for understanding the rest of this PR -- how the feature works, key components, decisions, and limitations.It also links to more detailed docs in the
docs/folder wherepipeline.md(more detailed notes + diagrams per component) andconcurrency.mdmight be useful reading.Refactoring is not too hard -- so if the PR would benefit from major changes, please ask!
2. Review by Commit
This PR is organized by commit. It's probably easiest to review by commit, and I'll stop rewriting history now that the PR is open and any fixes will be batched into future commits.
common,logger,resources, settings.DaemonProtocol.kt.LiveReloadOrchestrator.kt,ChangeClassifier.kt,SessionReducer.kt.DaemonMain.kt,IncrementalCompiler.kt.QuickBuildPlugin.kt,ProxySourceGenerator.kt,QuickBuildManifestTransformer.kt— manifest rewriting hides subtle breaks.QuickBuildRuntime.java,QuickBuildAppComponentFactory.java,PayloadStore.java.AndroidManifest.xml. Start fromQuickBuildAction.kt,QuickBuildModule.kt,GradleQuickBuildProvisioner.kt.3. How this was tested
quickbuild/docs/manual-qa.mdfor the test plan.So far we've run Block A + B and currently there are a few minor bugs left to fix, but it mostly works!See these two Loom videos::resources.4. Known limitations and next steps
This highlights some of the more important next steps and known limitations. Please see the limitations section of
quickbuild/README.mdfor a more complete list.Next steps to get Quick Build done
Existing tickets:
Known Limitations
android:processattribute yet🤖 Generated with Claude Code, edited heavily by Bryan
https://claude.ai/code/session_01CsRt7FJyQtTkkJoCcEURA9