diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/ApkInstaller.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/ApkInstaller.kt index 2ca2eb2..c1d3eb9 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/ApkInstaller.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/ApkInstaller.kt @@ -36,6 +36,18 @@ fun downloadApk( // second phone must not have its download changed by it. The // server checks the path against that component's builds, so a stale // one falls back to its newest rather than naming a file. + // This app's own APK is the one download that has to survive the + // server changing underneath it: the build that produced it rebuilt + // the server too, and the process answering afterwards is a newer one + // than the app asking. So the built-in project goes over the frozen + // rescue route rather than the manifest's, which is the same route + // and the same file the self-update check offers -- the difference is + // only that nothing about it can be renamed. It carries no component + // or variant because it cannot: the built-in project has one APK, and + // which build of it to serve is the mode set on the build machine. + if (entry.builtIn) { + return downloadFromRoute(context, SELF_APK_ROUTE, "self", onProgress) + } val query = StringBuilder("?component=").append(encode(component)) chosenVariant(context, entry.key, component)?.let { query.append("&variant=").append(encode(it)) diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt index 6421cc2..58c9e20 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -161,6 +161,17 @@ private const val REFRESH_ATTEMPTS_AFTER_PULL = 4 private const val WAKE_RETRY_MS = 600L private const val RESTART_WAIT_MS = 1000L +/** + * How many of those to wait through when the built-in project's own update is what is happening. + * + * Longer than [REFRESH_ATTEMPTS_AFTER_PULL] because this is not a retry of a read that might + * succeed anyway — it is waiting out a restart that is definitely happening, and one going through + * a service manager is a stop and a start rather than an exec. Bounded all the same: the APK is + * already downloaded by the second of the two waits, and never installing it would be the worse + * failure. + */ +private const val RESTART_ATTEMPTS = 15 + private sealed class ManifestState { data object Loading : ManifestState() @@ -311,12 +322,20 @@ fun UpdaterScreen(settingsVersion: Int) { // not -- which is exactly when replacing this app matters most. var selfUpdate by remember { mutableStateOf(null) } val selfUpdateContext = LocalContext.current - // On arrival, and again whenever this app's own project has just been - // built: the second is the moment the newer copy comes into existence, - // and the moment the server it has to keep talking to has just - // changed. - var selfUpdateTick by remember { mutableStateOf(0) } - LaunchedEffect(selfUpdateTick) { selfUpdate = selfUpdateAvailable(selfUpdateContext) } + // Offered when, and only when, the list cannot offer it: the built-in + // card is not there, or there is no list at all. That card's own + // Update now pulls, builds and installs this app like any other, and + // a dialog appearing over the card that is already doing it says the + // same thing twice -- worse, it says it in the one place a person + // cannot see what it is about. What is left is the case this screen + // was written for: a server that has changed what the manifest says + // into something this app is too old to read, which is exactly when + // replacing this app matters most and exactly when nothing on the + // list can say so. + var selfCardMissing by remember { mutableStateOf(false) } + LaunchedEffect(selfCardMissing) { + selfUpdate = if (selfCardMissing) selfUpdateAvailable(selfUpdateContext) else null + } // Keys of apps added on the Add screen, waiting to be taken into the // list one at a time. var added by remember { mutableStateOf>(emptyList()) } @@ -331,7 +350,7 @@ fun UpdaterScreen(settingsVersion: Int) { added = added, onAddedApplied = { added = emptyList() }, onAdd = { adding = true }, - onOwnProjectBuilt = { selfUpdateTick++ }, + onSelfCardMissing = { selfCardMissing = it }, ) selfUpdate?.let { build -> // Over the list for the same reason the Add screen is, and @@ -500,12 +519,26 @@ private fun AppListScreen( added: List, onAddedApplied: () -> Unit, onAdd: () -> Unit, - onOwnProjectBuilt: () -> Unit, + /** Whether the built-in project's card is unavailable to offer this app's own update. */ + onSelfCardMissing: (Boolean) -> Unit, ) { val context = LocalContext.current val scope = rememberCoroutineScope() var manifestState by remember { mutableStateOf(ManifestState.Loading) } + // Whether the built-in card is unavailable to offer this app's own + // update, which is the whole of what the screen above needs from this + // one. Derived here rather than asked for, so the rule sits beside the + // state it reads. Loading counts as present: not having found out yet + // is not the same as an answer, and a dialog that flashed up during + // every load would be exactly the noise it is there to avoid. + val selfCardMissing = + when (val state = manifestState) { + is ManifestState.Loaded -> state.manifest.entries.none { it.builtIn } + is ManifestState.Error -> true + ManifestState.Loading -> false + } + LaunchedEffect(selfCardMissing) { onSelfCardMissing(selfCardMissing) } // What each project is doing, and separately what each of its // components is: two levels, like installedTimes below and for the // same reason. A project can build two clients, and one of them being @@ -823,6 +856,31 @@ private fun AppListScreen( } } + /** + * Waits until the server is answering again, for a bounded time, and says whether it is. + * + * Only the built-in project needs it, and it needs it twice. Building this project rebuilds + * this server's binary and restarts it a moment later, so the two things that come after that + * build — fetching the new APK, and handing it to the system installer — both happen while the + * process on the other end may be exec-ing into its replacement. A download that dies mid + * transfer reads as the update having failed at the moment it was working, and an install + * offered during the restart replaces this app while the server it must talk to is down. + * + * Asked over `/self`, the same frozen route the rescue check uses: it is two numbers and no + * nesting, so it answers as soon as the new process is listening whatever else changed. + * + * Gives up rather than waiting for ever, and the caller carries on anyway: the APK is already + * on the phone by then, and refusing to install it because the server is down would strand + * somebody at the one moment a newer copy might be the fix. + */ + suspend fun awaitServerBack(): Boolean { + repeat(RESTART_ATTEMPTS) { + if (withContext(Dispatchers.IO) { runCatching { selfBuild() }.isSuccess }) return true + delay(RESTART_WAIT_MS) + } + return false + } + fun install(file: File) { if (!canRequestInstall(context)) { context.startActivity(requestInstallPermissionIntent(context)) @@ -851,13 +909,6 @@ private fun AppListScreen( try { applyOne(entry.key) settle() - // Building this app's own project is what produces the - // newer copy of it, and restarts the server it has to - // keep talking to. So this is the moment to offer it, - // rather than at some later launch. - if (entry.builtIn) { - onOwnProjectBuilt() - } return } catch (e: DownloadServerException) { if (attempt == REFRESH_ATTEMPTS_AFTER_PULL - 1) { @@ -986,6 +1037,17 @@ private fun AppListScreen( } } + // The build that just ran rebuilt this server, so what is + // about to be asked for the APK is a process that may be + // replacing itself. Waited for here rather than retried after + // the fact: a download that dies partway is reported as a + // failure, and this is the update where that failure reads as + // the update itself having broken. + if (entry.builtIn) { + setComponent(entry.key, component, ComponentState.Busy("Waiting for the server")) + awaitServerBack() + } + // Not "downloading" until something is actually coming down: // the server may still be producing what it is about to send. setComponent(entry.key, component, ComponentState.Fetching) @@ -1026,6 +1088,16 @@ private fun AppListScreen( setComponent(entry.key, component, ComponentState.ReadyToInstall(file)) return@run false } + // The other half of the wait above: the download itself holds + // the restart off while it runs -- the server counts what it + // is sending -- so the exec lands, if it lands at all, in the + // moment between the last byte and this install. Replacing + // this app then leaves the copy that starts next unable to + // reach anything, which reads as the update having broken it. + if (entry.builtIn) { + setComponent(entry.key, component, ComponentState.Busy("Waiting for the server")) + awaitServerBack() + } setComponent(entry.key, component, null) install(file) true diff --git a/server/src/registry.rs b/server/src/registry.rs index 749185c..ee0c37f 100644 --- a/server/src/registry.rs +++ b/server/src/registry.rs @@ -319,58 +319,58 @@ fn component_id(component: &Component) -> ComponentId { ) } -/// What this machine chose about each component, for carrying across a -/// rewrite of the components list. +/// Everything about a component that this machine either measured or was +/// told here, for carrying across a rewrite of the components list. /// -/// The mirror of [`measured_packages`], and here for the same reason: -/// both acceptance and the self entry's startup reconciliation replace -/// `components` wholesale with what the checkout declares, which is right -/// for everything the project asked for and wrong for everything somebody -/// chose here. Without it, choosing to build this server in `debug` -/// would last exactly until the next restart -- and restarting is how -/// this server is updated, so it would never last at all. +/// There are two places that rewrite one -- accepting a declaration, and +/// the built-in row's reconciliation at every startup -- and both replace +/// `components` wholesale with what the checkout declares. That is right +/// for everything the project asked for and wrong for everything this +/// side knows: the package read out of an APK, the mode somebody picked, +/// the commit a build recorded. One type for all three rather than a +/// collect-and-restore pair each, because the pairs were remembered +/// individually and one of them was not -- acceptance carried the package +/// and the chosen mode and dropped `builtFrom`, so accepting a project's +/// declaration told every one of its cards it had never been built here. +/// Silent, because that is a true sentence about plenty of projects and +/// so reads as an answer rather than as a loss. Adding a field to a +/// component now means adding it here once, and both callers get it. /// -/// Keyed the same way for the same reason: once a component's directory -/// decides which builds are its own, a reused name is a different -/// component, and handing it the old one's mode would build something -/// nobody asked for. -fn chosen_settings(components: &[Component]) -> HashMap { - components - .iter() - .map(|component| (component_id(component), component.choices())) - .collect() +/// Keyed by name *and* directory like everything else here: once a +/// component's directory decides which builds are its own, a reused name +/// is a different component, and handing it the old one's package or +/// commit would be wrong until that component happened to be built. +#[derive(Default)] +struct CarriedOver { + packages: HashMap, + chosen: HashMap, + built: HashMap, Option)>, } -fn restore_chosen_settings( - components: &mut [Component], - chosen: &HashMap, -) { - for component in components { - if let Some(choices) = chosen.get(&component_id(component)) { - component.restore(choices.clone()); - } +fn carried_over(components: &[Component]) -> CarriedOver { + CarriedOver { + packages: measured_packages(components), + chosen: components + .iter() + .map(|component| (component_id(component), component.choices())) + .collect(), + built: components + .iter() + .map(|component| (component_id(component), component.built())) + .collect(), } } -/// What a build recorded about each component, keyed the way every other -/// carry-across here is: by name *and* directory, so a reused name for a -/// component that now builds somewhere else does not inherit the old one's -/// commit. -fn built_records( - components: &[Component], -) -> HashMap, Option)> { - components - .iter() - .map(|component| (component_id(component), component.built())) - .collect() -} - -fn restore_built( - components: &mut [Component], - recorded: &HashMap, Option)>, -) { +fn restore_carried_over(components: &mut [Component], carried: &CarriedOver) { for component in components { - if let Some(built) = recorded.get(&component_id(component)) { + let id = component_id(component); + if let Some(package) = carried.packages.get(&id) { + component.set_package(package.clone()); + } + if let Some(choices) = carried.chosen.get(&id) { + component.restore(choices.clone()); + } + if let Some(built) = carried.built.get(&id) { component.set_built(built.clone()); } } @@ -793,11 +793,14 @@ impl AppState { /// thing that rewrites that file is a pull the same person just /// performed. /// - /// Measured values are carried across rather than reset. A declaration - /// says nothing about the package a build produces -- that is read from - /// the APK -- so accepting one must not throw away what was already - /// read, or every acceptance would blank the installed-version check - /// until the next build. + /// What this machine measured or was told is carried across rather + /// than reset, through [`CarriedOver`]. A declaration says nothing + /// about the package a build produces, the mode somebody picked here, + /// or the commit a build was made from -- so accepting one must not + /// throw any of them away. Dropping the package would blank the + /// installed-version check until the next build; dropping the commit + /// did worse, because a component with nothing recorded reads as one + /// this server has never built. pub fn approve_declaration(&self, key: &str) -> Result<()> { let entry = self .entry(key) @@ -816,8 +819,7 @@ impl AppState { .iter_mut() .find(|project| project.key == key) .with_context(|| format!("{key} is built in and has nothing to accept"))?; - let measured = measured_packages(&project.components); - let chosen = chosen_settings(&project.components); + let carried = carried_over(&project.components); project.git_pull = declared.git_pull; // Everything `matches_accepted` compares has to be written // here, or accepting cannot clear the gate. `resources` joined @@ -828,17 +830,12 @@ impl AppState { // nothing. Add a field to the gate and add it here. project.resources = declared.resources; project.components = declared.components; - for component in &mut project.components { - if let Some(package) = measured.get(&component_id(component)) { - component.set_package(package.clone()); - } - } // Accepting is about what the project asks to have run. It is // not somebody withdrawing the mode they picked or the strip - // they turned off, so those survive it -- exactly as the - // measured package does, and for the same reason: neither is - // part of what was being agreed to. - restore_chosen_settings(&mut project.components, &chosen); + // they turned off, and it is not this machine forgetting what + // it built -- none of those is part of what was being agreed + // to. + restore_carried_over(&mut project.components, &carried); Ok(()) }) } @@ -920,29 +917,28 @@ fn reconcile_self(config: &mut Config, self_project: &Path) -> bool { "app".to_string() } }; - set_measured_package(&mut components, &apk_name, SELF_PACKAGE.to_string()); let existing = config .projects .iter_mut() .find(|project| project.key == SELF_KEY); // Carried across the row being rebuilt, like `gitIpv4` below: what is - // *derived* here is what the checkout declares, and a mode chosen for - // this server's own component is not that. Restarting is how this - // server takes an update, so a choice that did not survive a restart - // would not survive being acted on. + // *derived* here is what the checkout declares, and neither a mode + // chosen for this server's own component nor the commit its last + // build was made from is that. It matters more here than at an + // acceptance, because this row is re-derived at *every* startup and + // restarting is how this server takes its own update -- so a choice + // that did not survive a restart would not survive being acted on, + // and a commit that did not left this server's own card with no + // freshness for ever after: never behind, never current, and its + // Update with nothing to do, since both readings need a commit to + // compare against. if let Some(existing) = existing.as_ref() { - restore_chosen_settings(&mut components, &chosen_settings(&existing.components)); - // And what a build here measured, for the same reason and with - // more force: this row is re-derived at *every* startup, and - // restarting is how this server takes its own update -- so a - // commit recorded by the build that produced the new binary was - // forgotten by the process it started. Its own card then had no - // freshness for ever after: never behind, never current, and its - // Update with nothing to do, because both readings need a commit - // to compare against. Carried across rather than re-derived, - // because nothing but a build can know it. - restore_built(&mut components, &built_records(&existing.components)); + restore_carried_over(&mut components, &carried_over(&existing.components)); } + // After the carry-across, so what is derived wins: this is the one + // project whose package this server states rather than reads, and an + // older row carrying something else must not override it. + set_measured_package(&mut components, &apk_name, SELF_PACKAGE.to_string()); let derived = ProjectConfig { key: SELF_KEY.to_string(), label: declared.label.unwrap_or_else(|| SELF_LABEL.to_string()), @@ -1379,6 +1375,36 @@ mod tests { assert_eq!(component.build().to_line(), "d"); } + /// The commit a build recorded is a measurement of this machine, not + /// something the project's declaration has an opinion about -- so it + /// survives that declaration being accepted again, exactly as the + /// chosen mode and the measured package do. It did not, and the cost + /// was invisible: with nothing recorded a component reads as one this + /// server has never built, which is a true sentence about plenty of + /// projects and so looks like an answer rather than a loss. Every + /// card of a project whose declaration had ever been accepted said it. + #[test] + fn what_a_build_recorded_survives_the_declaration_being_accepted_again() { + let dir = tempfile::tempdir().expect("tempdir"); + write_moded_request(dir.path()); + let (_home, state) = state_with(dir.path()); + state.approve_declaration("demo").expect("approve"); + state.record_built("demo", "app", "c0ffee".to_string()); + + state.approve_declaration("demo").expect("approve again"); + let component = &state.entry("demo").expect("entry").components[0]; + assert_eq!( + component.built_from(), + Some("c0ffee"), + "accepting must not tell the card this component was never built here", + ); + assert_eq!( + component.built_mode(), + Some("release"), + "and the mode goes with the commit, or the record describes a build nobody made", + ); + } + /// A mode this component does not declare is refused rather than /// stored: `ByMode::get` falls back to the first, so storing it would /// leave the card naming a mode nothing is built in -- which reads