diff --git a/AGENTS.md b/AGENTS.md index 0995e37..69a0624 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -388,17 +388,56 @@ mutable at runtime from the phone. the time one after the other does. The saving only appears when more than one has work, which is what a pull produces. The status is per component (`ComponentStatus`), so a card draws each - component's bar, timing and last line inside that component's own row; + component's bar and last line inside that component's own row; the project's own area at the bottom keeps only what belongs to the whole project, which is fetching and pulling. A bar under the card could only ever say that *something* was happening, and with everything building at once that is exactly what the reader is trying to find out. - A failure no longer stops the others -- they are already running -- so - the first failure in declaration order is the one reported, which is what - a walk in that order would have said. The phone draws every component in + A failure no longer stops the others -- they are already running -- and + it is reported against the component whose command it was, never as the + project's. The phone draws every component in a card of its own, including a project with only one: the flat layout it used to get meant two shapes to keep in step, and put that APK's size up beside the card's corner controls where it read as belonging to them. +- **There is a build slot per component, not per project, and the two + halves have to agree on that.** A component being built neither blocks + another's build nor disables its controls: `Inner` has no `building` + flag, only a `ComponentRun` per component whose open `step` *is* the + answer, and `claim` writes that entry synchronously under the same lock + the route answers from -- so nothing can read a component the request + just claimed as idle, which the phone would take for "the build is + over". `RunningBuild::building` stays, but it means "anything at all is + happening here" and is only for the controls that act on the whole + checkout; anything about one component reads that component's `step`. + The app mirrors the split exactly: `ProjectState` for the pull and the + project-wide Rebuild, `ComponentState` keyed by component name for + everything a single component is asked to do. One map keyed by project + alone is what the bug was -- pressing Update on one client of a + two-client project disabled the other's button and drew this one's + download bar under it -- and two hierarchies rather than one keyed by a + pair is what stops it coming back, since a download has no + project-wide meaning to be stored with. + The exception, and it is worth keeping visible so it does not read as + more of the same: **a pull really is exclusive with everything.** There + is one checkout, and it rewrites the files every component builds from, + so `Inner::pulling` blocks any component from being claimed and waits + for any still building. Ending the pull and claiming what it decided to + build happen under one lock for the same reason `claim` is + synchronous -- a phone polling in the gap would see a project that is + neither pulling nor building and call the run finished. +- **A finished component shows nothing, and its button goes back to + normal.** Iris's call, 2026-09-01: "you shouldn't see the time it took + once it finishes, it should just go back to its normal enabled button + state." So `ComponentBuildProgress` draws only while the step is open, + and the elapsed times are gone from both halves of the wire -- a bar, a + count and a last line all describe something happening *now*, and every + one of them sits there looking live beside a sibling that genuinely is. + What says the build landed is the control becoming pressable again and + the card's own freshness. The failure is the exception, because it is an + outcome rather than residue; it is drawn by the component card next to + the Retry that acts on it, in the one place that reports that + component's failures whether they came from a build, a download or a + service action. - **A project's own `.dev-updater.ron` is a request, never an instruction.** It only runs once accepted from the phone, which copies it into `config.ron`; `AppEntry::pending_declaration` is the whole gate. diff --git a/README.md b/README.md index 2dc1c97..620ee20 100644 --- a/README.md +++ b/README.md @@ -168,12 +168,18 @@ Pull acts on the build machine — fetch, fast-forward, then run the command — while **Update** still means "install what's built onto this phone", so the two never mean each other. -While it runs, the card says which step is happening, how long it has -taken, what the finished steps took, and the last line the build printed. -The command's output is read as it arrives rather than collected at the -end, so a long build is visibly moving instead of being indistinguishable -from a stuck one — and when it *is* slow, the phase timings say whether -the time went to the network or the compiler. +While it runs, each component's own row says which step is happening and +the last line that component printed. The command's output is read as it +arrives rather than collected at the end, so a long build is visibly +moving instead of being indistinguishable from a stuck one. When it +finishes, all of that goes and the button becomes pressable again: what a +row shows is work in progress, not a report on work that is over. + +Components of one project build at the same time and are independent of +each other, so updating one client of a project that builds two leaves the +other's button live rather than making it wait. A pull is the exception — +there is one checkout, so it waits for everything and everything waits for +it. Deliberate limits, because a phone is a bad place to resolve a mess: diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt index 825fabe..2fc2e78 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt @@ -16,7 +16,17 @@ import org.json.JSONObject // (needsBuild / canPull). data class BuildStatus( val stale: Boolean, + // Anything at all is happening for this project -- a pull, or any one + // of its components being built. The *project's* question, for the + // controls that act on the whole checkout. Anything about one + // component asks [component] instead and reads its `running`: this one + // says yes while a sibling builds, and waiting on it is what used to + // put one client's download behind the other client's build. val building: Boolean, + // Why the last pull could not be made. Pulls only -- a build failure + // belongs to the component whose command it was, and is in + // [ComponentBuild.error], because components build at once and two of + // them can fail differently. val error: String?, // The failure above was a pull with no fast-forward to make, because // the checkout on the build machine shares no commit with its @@ -25,11 +35,10 @@ data class BuildStatus( // wording is translated, so a button that matched on it would appear // only on an English build machine. val unrelatedHistories: Boolean, - // What the whole *project* is doing ("fetching", "pulling"), and how - // long this run has taken. Work belonging to one component is in - // [components] instead, because that is where it is drawn. + // What the whole *project* is doing ("fetching", "pulling"). Work + // belonging to one component is in [components] instead, because that + // is where it is drawn. val phase: String?, - val elapsedMs: Long, val components: List, ) { /** This component's part of the run, if it has reached it yet. */ @@ -48,7 +57,6 @@ data class ComponentBuild( // What it is doing now ("building", "installing", "restarting"), or // null once it has finished. val step: String?, - val elapsedMs: Long, val progress: BuildProgressCount?, val log: List, val error: String?, @@ -76,7 +84,6 @@ private fun requestBuildStatus(path: String, method: String): BuildStatus = error = if (json.isNull("error")) null else json.getString("error"), unrelatedHistories = json.optBoolean("unrelatedHistories", false), phase = if (json.isNull("phase")) null else json.optString("phase").ifEmpty { null }, - elapsedMs = json.optLong("elapsedMs", 0), components = (0 until (components?.length() ?: 0)).map { index -> val component = components!!.getJSONObject(index) @@ -86,7 +93,6 @@ private fun requestBuildStatus(path: String, method: String): BuildStatus = step = if (component.isNull("step")) null else component.optString("step").ifEmpty { null }, - elapsedMs = component.optLong("elapsedMs", 0), progress = component.optJSONObject("progress")?.let { BuildProgressCount( 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 df2c9f2..5752a13 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -153,13 +153,26 @@ private sealed class ManifestState { data class Error(val message: String) : ManifestState() } -private sealed class CardState { +/** + * What a whole *project* is being made to do, as opposed to one of its components. + * + * Only the things that act on the checkout every component is built from, which is why they are the + * project's: a pull rewrites it, and Rebuild runs every component's command. Anything that belongs + * to one component is a [ComponentState] instead — held in a map keyed by component name, so + * starting one component's work cannot disable, describe or fail another's. + * + * Two hierarchies rather than one keyed by a pair, because they are not the same set of states: a + * download has no project-wide meaning and a pull has no component-wide one, and keeping them apart + * is what stops a component's progress being stored where every component would read it. That was + * the bug — one map keyed by project alone, so pressing Update on one client of a two-client + * project disabled the other's button and drew this one's download bar under it. + */ +private sealed class ProjectState { /** * [status] is the server's live progress, refreshed on every poll, so the card can say which - * step is running and how long it has taken rather than showing an unchanging spinner for the - * length of a build. + * step is running rather than showing an unchanging spinner for the length of a build. */ - data class Pulling(val status: BuildStatus?) : CardState() + data class Pulling(val status: BuildStatus?) : ProjectState() /** * A build somebody asked for outright, as opposed to one a pull or a download brought about. @@ -167,9 +180,23 @@ private sealed class CardState { * nothing was pulled here, and a bar that says otherwise is the kind of small lie that makes a * reader stop trusting the rest. */ - data class Rebuilding(val status: BuildStatus?) : CardState() + data class Rebuilding(val status: BuildStatus?) : ProjectState() - data class Preparing(val status: BuildStatus?) : CardState() + /** + * Why the last thing asked of the whole project stopped. + * + * There is no "retry" recorded with it, because the control that would redo it is the one + * sitting beside the message: a project's actions are Pull and Rebuild, both in the card's own + * row, and both re-enabled by a failure. A component's failure is a [ComponentState.Error] and + * gets Retry on its own row, which is what stops a failed pull being retried as a download. + */ + data class Error(val message: String) : ProjectState() +} + +/** What one component of a project is being made to do. */ +private sealed class ComponentState { + /** The build machine is producing this component's build, before it can be downloaded. */ + data class Preparing(val status: BuildStatus?) : ComponentState() /** * The request is out and the server hasn't started sending. For an app served as a stripped @@ -179,19 +206,30 @@ private sealed class CardState { * Named for the transport rather than "preparing", which is taken by the configured build step * above; what a person reads is "preparing" either way. */ - data object Fetching : CardState() + data object Fetching : ComponentState() /** [progress] is null when the response gave no length to measure against. */ - data class Downloading(val progress: Float?) : CardState() + data class Downloading(val progress: Float?) : ComponentState() - /** - * [retryPull] records which action failed, because Retry has to redo that one: a failed pull - * retried as a download would silently do something else than the button that produced the - * error. - */ - data class Error(val message: String, val retryPull: Boolean = false) : CardState() + /** Why the last thing this component was asked to do stopped. */ + data class Error(val message: String) : ComponentState() } +/** + * Whether this is work in progress, as opposed to the record of work that has stopped. + * + * The distinction every "should this control be disabled" question wants, and the reason a failure + * is a state here rather than a field on one: an error is something to read, not something to wait + * for, so a card holding one is idle. Held as a property of the state rather than tested at each + * site, because there are five of those and a missed one leaves a button dead until the screen is + * reloaded. + */ +private val ProjectState?.busy: Boolean + get() = this is ProjectState.Pulling || this is ProjectState.Rebuilding + +private val ComponentState?.busy: Boolean + get() = this != null && this !is ComponentState.Error + /** How far the list is dimmed behind a modal. One value, since two of them dim it. */ private const val SCRIM_ALPHA = 0.6f @@ -417,7 +455,15 @@ private fun AppListScreen( val scope = rememberCoroutineScope() var manifestState by remember { mutableStateOf(ManifestState.Loading) } - var cardStates by remember { mutableStateOf>(emptyMap()) } + // 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 + // updated says nothing about the other -- keyed by project alone, it + // said it about both. + var projectStates by remember { mutableStateOf>(emptyMap()) } + var componentStates by remember { + mutableStateOf>>(emptyMap()) + } // Written only by updateInstalledState below, called from either of the // two effects that follow -- so a fresh manifest, a package-change // broadcast, and a return from the system installer all go through the @@ -443,6 +489,26 @@ private fun AppListScreen( // that is busy is the one that shows it. var serviceBusy by remember { mutableStateOf>(emptyMap()) } + /** + * Puts one component's state, or takes it away for null. + * + * The one place the two-level map is written, so a write for one component cannot drop the + * entry another one is keeping. + */ + fun setComponent(key: String, component: String, state: ComponentState?) { + val forProject = componentStates[key] ?: emptyMap() + val updated = + when (state) { + null -> forProject - component + else -> forProject + (component to state) + } + componentStates = + when { + updated.isEmpty() -> componentStates - key + else -> componentStates + (key to updated) + } + } + /** * Replaces the list without showing it as loading, and keeps looking while the server says a * remote check is still running. @@ -486,7 +552,8 @@ private fun AppListScreen( fun refreshByPull() { if (pulling) return pulling = true - cardStates = emptyMap() + projectStates = emptyMap() + componentStates = emptyMap() scope.launch { manifestState = try { @@ -500,7 +567,8 @@ private fun AppListScreen( fun refresh() { manifestState = ManifestState.Loading - cardStates = emptyMap() + projectStates = emptyMap() + componentStates = emptyMap() scope.launch { manifestState = try { @@ -596,7 +664,8 @@ private fun AppListScreen( try { awaitCheck(entry.key) } catch (e: DownloadServerException) { - cardStates = cardStates + (entry.key to CardState.Error(e.message ?: "Failed")) + projectStates = + projectStates + (entry.key to ProjectState.Error(e.message ?: "Failed")) } } } @@ -620,27 +689,40 @@ private fun AppListScreen( */ suspend fun followBuild( entry: ManifestEntry, - retryPull: Boolean, start: suspend () -> BuildStatus, - progress: (BuildStatus?) -> CardState, + progress: (BuildStatus?) -> ProjectState, ) { - cardStates = cardStates + (entry.key to progress(null)) + projectStates = projectStates + (entry.key to progress(null)) try { var status = withContext(Dispatchers.IO) { start() } while (status.building) { - cardStates = cardStates + (entry.key to progress(status)) + projectStates = projectStates + (entry.key to progress(status)) delay(BUILD_POLL_INTERVAL_MS) status = withContext(Dispatchers.IO) { buildStatus(entry.key) } } + // A failure is reported where it happened. The project's own + // error is the pull's -- there is one checkout and one thing + // that could have gone wrong with it -- while a build that + // fell over belongs to the component whose command it was, + // since a run builds every component at once and two of them + // can fail differently. val failure = status.error if (failure != null) { - cardStates = cardStates + (entry.key to CardState.Error(failure, retryPull)) + projectStates = projectStates + (entry.key to ProjectState.Error(failure)) // The card keeps the message either way -- the dialog is // dismissible, and a failure that vanished with it would // leave the card looking as though nothing had happened. if (status.unrelatedHistories) forcePull = entry return } + // Kept after the project's own state is cleared below, so a + // component that failed still says so once the run it was + // part of is over. + for (component in status.components) { + component.error?.let { + setComponent(entry.key, component.name, ComponentState.Error(it)) + } + } // The APK's mtime is what decides "update available", so the // list has to come from the server again rather than be // guessed at here. @@ -659,7 +741,11 @@ private fun AppListScreen( repeat(REFRESH_ATTEMPTS_AFTER_PULL) { attempt -> try { applyOne(entry.key) - cardStates = cardStates - entry.key + // The project's own state only. A component that + // failed inside this run has just been given its own, + // and clearing that here would take away the only + // thing saying so. + projectStates = projectStates - entry.key // 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, @@ -670,7 +756,7 @@ private fun AppListScreen( return } catch (e: DownloadServerException) { if (attempt == REFRESH_ATTEMPTS_AFTER_PULL - 1) { - cardStates = cardStates - entry.key + projectStates = projectStates - entry.key manifestState = ManifestState.Error(e.message ?: "Unknown error") } else { delay(RESTART_WAIT_MS) @@ -678,8 +764,7 @@ private fun AppListScreen( } } } catch (e: DownloadServerException) { - cardStates = - cardStates + (entry.key to CardState.Error(e.message ?: "Failed", retryPull)) + projectStates = projectStates + (entry.key to ProjectState.Error(e.message ?: "Failed")) } } @@ -693,9 +778,8 @@ private fun AppListScreen( scope.launch { followBuild( entry, - retryPull = true, start = { pullAndBuild(entry.key, force) }, - progress = { CardState.Pulling(it) }, + progress = { ProjectState.Pulling(it) }, ) } } @@ -708,55 +792,82 @@ private fun AppListScreen( scope.launch { followBuild( entry, - retryPull = false, start = { buildNow(entry.key) }, - progress = { CardState.Rebuilding(it) }, + progress = { ProjectState.Rebuilding(it) }, ) } } + /** + * Builds one component if it needs it, downloads it, and hands it to the installer. + * + * Every step of it is recorded against that component and nothing else. This is the action the + * whole two-level map exists for: a project can build two independent clients, and updating one + * of them must leave the other's button pressable and its row silent — with one slot per + * project, this one's bar was drawn under both and both buttons went dead. + */ fun startUpdate(entry: ManifestEntry, component: String) { scope.launch { if (entry.needsBuild) { - cardStates = cardStates + (entry.key to CardState.Preparing(null)) + setComponent(entry.key, component, ComponentState.Preparing(null)) try { var status = withContext(Dispatchers.IO) { prepareBuild(entry.key, component) } - while (status.building) { - cardStates = cardStates + (entry.key to CardState.Preparing(status)) + // This component's own step, not the project's + // `building`: a sibling being built at the same time + // says yes to that one, and waiting on it would put + // this download behind a build it has nothing to do + // with -- which is the coupling this is here to end. + var started = false + while (status.component(component)?.running == true) { + started = true + setComponent(entry.key, component, ComponentState.Preparing(status)) delay(BUILD_POLL_INTERVAL_MS) status = withContext(Dispatchers.IO) { buildStatus(entry.key) } } - val buildError = status.error + // Only from a build this press actually started. The + // server keeps a component's last outcome until it is + // built again, so an older failure is still sitting + // there -- and reading that one would refuse a + // download because of something already dealt with. + val buildError = status.component(component)?.error?.takeIf { started } if (buildError != null) { - cardStates = cardStates + (entry.key to CardState.Error(buildError)) + setComponent(entry.key, component, ComponentState.Error(buildError)) return@launch } } catch (e: DownloadServerException) { - cardStates = - cardStates + - (entry.key to - CardState.Error(e.message ?: "Couldn't prepare phone build")) + setComponent( + entry.key, + component, + ComponentState.Error(e.message ?: "Couldn't prepare phone build"), + ) return@launch } } // Not "downloading" until something is actually coming down: // the server may still be producing what it is about to send. - cardStates = cardStates + (entry.key to CardState.Fetching) + setComponent(entry.key, component, ComponentState.Fetching) val file = try { withContext(Dispatchers.IO) { downloadApk(context, entry, component) { read, total -> val progress = if (total > 0) read.toFloat() / total else null - cardStates = cardStates + (entry.key to CardState.Downloading(progress)) + setComponent( + entry.key, + component, + ComponentState.Downloading(progress), + ) } } } catch (e: DownloadServerException) { - cardStates = - cardStates + (entry.key to CardState.Error(e.message ?: "Download failed")) + setComponent( + entry.key, + component, + ComponentState.Error(e.message ?: "Download failed"), + ) return@launch } - cardStates = cardStates - entry.key + setComponent(entry.key, component, null) install(file) } } @@ -770,10 +881,11 @@ private fun AppListScreen( withContext(Dispatchers.IO) { action() } // This card's state only: another card's error is its own // and has nothing to do with what just happened here. - cardStates = cardStates - entry.key + projectStates = projectStates - entry.key if (removes) dropEntry(entry.key) else applyOne(entry.key) } catch (e: DownloadServerException) { - cardStates = cardStates + (entry.key to CardState.Error(e.message ?: "Failed")) + projectStates = + projectStates + (entry.key to ProjectState.Error(e.message ?: "Failed")) } } } @@ -798,20 +910,23 @@ private fun AppListScreen( withContext(Dispatchers.IO) { serviceAction(entry.key, component, action, purge) } - // Reported on the card the button was pressed on, because + // Reported in the row the button was pressed in, because // the request succeeded -- the service is gone -- and what // is left to say is that something is still on the build - // machine, which nobody can see from here otherwise. - cardStates = - if (result.leftBehind.isEmpty()) { - cardStates - entry.key - } else { - cardStates + - (entry.key to CardState.Error(result.leftBehind.joinToString("\n"))) - } + // machine, which nobody can see from here otherwise. On + // the component rather than the card: a project can run + // more than one service, and this is about one of them. + setComponent( + entry.key, + component, + when { + result.leftBehind.isEmpty() -> null + else -> ComponentState.Error(result.leftBehind.joinToString("\n")) + }, + ) applyOne(entry.key) } catch (e: DownloadServerException) { - cardStates = cardStates + (entry.key to CardState.Error(e.message ?: "Failed")) + setComponent(entry.key, component, ComponentState.Error(e.message ?: "Failed")) } finally { serviceBusy = serviceBusy - entry.key } @@ -1020,7 +1135,10 @@ private fun AppListScreen( // is not current yet. val (needAttention, upToDate) = entries.partition { entry -> - cardStates[entry.key].isBuilding() || + isBuilding( + projectStates[entry.key], + componentStates[entry.key], + ) || !entry.built || entry.newCommits || // Any client of the project being @@ -1054,7 +1172,8 @@ private fun AppListScreen( installedTimes = installedTimes[entry.key] ?: emptyMap(), chosenVariants = chosenVariants[entry.key] ?: emptyMap(), installedSizes = installedSizes[entry.key] ?: emptyMap(), - cardState = cardStates[entry.key], + projectState = projectStates[entry.key], + componentStates = componentStates[entry.key] ?: emptyMap(), onUpdate = { updated, component -> startUpdate(updated, component) }, @@ -1195,7 +1314,10 @@ private fun AppCard( installedTimes: Map, installedSizes: Map, chosenVariants: Map, - cardState: CardState?, + /** What the whole project is doing: pulling, rebuilding, or why one of those failed. */ + projectState: ProjectState?, + /** What each of its components is doing, by component name. */ + componentStates: Map, onUpdate: (ManifestEntry, component: String) -> Unit, onPull: () -> Unit, onRebuild: () -> Unit, @@ -1377,6 +1499,9 @@ private fun AppCard( val installed = installedTimes[component.name] val installedSize = installedSizes[component.name] val chosenVariantPath = chosenVariants[component.name] + // This component's own, so nothing below can + // reach for a sibling's by accident. + val componentState = componentStates[component.name] val upToDate = component.apk?.let { isUpToDate(it, installed, chosenVariantPath) @@ -1406,14 +1531,21 @@ private fun AppCard( // Stopping or uninstalling it is the one action // here that cannot be undone from the phone. isOwnServer = entry.builtIn, - build = componentBuild(cardState, component.name), + build = + componentBuild( + projectState, + componentState, + component.name, + ), // Being worked on right now, which the // component's own slice of the build says // directly rather than being inferred from a // project-wide phase name. working = - componentBuild(cardState, component.name)?.running == true, + componentBuild(projectState, componentState, component.name) + ?.running == true, busy = serviceBusy == component.name, + state = componentState, onAction = { action, purge -> onServiceAction(component.name, action, purge) }, @@ -1433,11 +1565,11 @@ private fun AppCard( needsBuild = entry.needsBuild, installed = installed != null, upToDate = upToDate, - cardState = cardState, + state = componentState, + projectState = projectState, onUpdate = { onUpdate(entry, component.name) }, - onPull = onPull, ) - ApkProgress(cardState) + ApkProgress(componentState) } }, ) @@ -1470,6 +1602,14 @@ private fun AppCard( // then a card with no Pull reads the same whether the remote // had nothing or was never asked. A failed check enables it // again, since pressing Pull is how you find out. + // Both of the buttons below act on the whole checkout, so they + // really do wait on everything: a pull rewrites the files every + // component is built from, and Rebuild runs every command. That + // is the project's own coupling rather than the components' -- + // it is why the server refuses them while anything is running, + // and why disabling them here is honest where disabling a + // component's own Update button was not. + val projectBusy = projectState.busy || componentStates.values.any { it.busy } Row( verticalAlignment = Alignment.CenterVertically, modifier = Modifier.fillMaxWidth(), @@ -1485,8 +1625,19 @@ private fun AppCard( if (entry.canPull && !awaitingApproval) { TextButton( onClick = onPull, + // A failed check enables it, since pressing Pull is + // how you find out -- and so does a failed pull, + // for the same reason and because this is now the + // only control that redoes one. A fetch that fell + // over leaves `newCommits` saying whatever the last + // successful check said, which for a project nobody + // had checked yet is "nothing", so without this the + // message would sit above a button that could not + // be pressed to answer it. enabled = - (entry.newCommits || entry.checkError != null) && cardState == null, + (entry.newCommits || + entry.checkError != null || + projectState is ProjectState.Error) && !projectBusy, colors = ActionTone.Primary.colors(), ) { Text("Pull & Build") @@ -1499,7 +1650,7 @@ private fun AppCard( Spacer(Modifier.weight(1f)) TextButton( onClick = onRebuild, - enabled = cardState == null, + enabled = !projectBusy, // The colour Restart and Reinstall wear: it // certainly does something, and what it leaves // behind is not obvious from here. @@ -1535,24 +1686,26 @@ private fun AppCard( // APK's bar sits under its button: a bar reports on a control, // and one placed away from it belongs to nothing in particular. // This had gone missing entirely when Pull moved up here. - if (cardState is CardState.Pulling) { - BuildProgress("Pulling and building", cardState.status) + if (projectState is ProjectState.Pulling) { + BuildProgress("Pulling and building", projectState.status) } - if (cardState is CardState.Rebuilding) { - BuildProgress("Building", cardState.status) + if (projectState is ProjectState.Rebuilding) { + BuildProgress("Building", projectState.status) } // What is left to say about the card once its components have - // said their own part: a failure, or that there is no build to - // talk about yet. Progress is not here -- it belongs beside the - // button that asked for it, which is in the APK's own card. + // said their own part: a failure of the project's own, or that + // there is no build to talk about yet. Progress is not here -- + // it belongs beside the button that asked for it, which is in + // the APK's own card. Nor is a component's failure, which is + // drawn in that component's row for the same reason. // // A failure is checked first: a project being built for the // first time is both at once, and the failure is the more // useful of the two. when { - cardState is CardState.Error -> - Text(cardState.message, color = MaterialTheme.colorScheme.error) + projectState is ProjectState.Error -> + Text(projectState.message, color = MaterialTheme.colorScheme.error) !entry.built && !awaitingApproval -> Text( @@ -1575,7 +1728,7 @@ private fun AppCard( // Pull asks the same remote the check asked, so the two say the // same paragraph twice, and the one that ran because somebody // pressed a button is the one they are waiting to read. - if (cardState !is CardState.Error) { + if (projectState !is ProjectState.Error) { entry.checkError?.let { reason -> Text( reason, @@ -1652,18 +1805,18 @@ private fun ProjectSettingsDialog( * started it, and one placed away from that control belongs to nothing in particular. */ @Composable -private fun ApkProgress(cardState: CardState?) { - when (cardState) { - is CardState.Preparing -> BuildProgress("Building for phone", cardState.status) +private fun ApkProgress(state: ComponentState?) { + when (state) { + is ComponentState.Preparing -> BuildProgress("Building for phone", state.status) - is CardState.Fetching -> { + is ComponentState.Fetching -> { ProgressBar() Spacer(Modifier.height(4.dp)) Text("Preparing the download...") } - is CardState.Downloading -> { - val progress = cardState.progress + is ComponentState.Downloading -> { + val progress = state.progress if (progress == null) { ProgressBar() } else { @@ -1699,9 +1852,14 @@ private fun UpdateButton( /** Whether this phone has the app at all, which decides "Install". */ installed: Boolean, upToDate: Boolean, - cardState: CardState?, + /** What this component is doing, which is what decides whether the button can be pressed. */ + state: ComponentState?, + /** + * What the whole project is doing, which also decides it — but only for the two things that act + * on the whole checkout. A sibling component being built is deliberately not in here. + */ + projectState: ProjectState?, onUpdate: () -> Unit, - onPull: () -> Unit, ) { // A project with nothing built still gets a button when its (accepted) // build step is what would produce the first APK -- otherwise adding it @@ -1709,13 +1867,11 @@ private fun UpdateButton( // way to do it. if (!built && !needsBuild) return - if (cardState is CardState.Error) { - TextButton( - onClick = { if (cardState.retryPull) onPull() else onUpdate() }, - colors = ActionTone.Primary.colors(), - ) { - Text("Retry") - } + // Retry redoes what failed, and what failed here was this component's + // own update -- a pull's failure is the project's and is retried from + // the project's own row. + if (state is ComponentState.Error) { + TextButton(onClick = onUpdate, colors = ActionTone.Primary.colors()) { Text("Retry") } return } @@ -1740,9 +1896,16 @@ private fun UpdateButton( // that reports it. A control that disappears takes the reader's // bearings with it, and what it says is still what pressing it would // have done. + // + // What counts as "running" is this component's own work, plus the two + // project-wide actions that would rebuild it out from under this + // press. A *sibling* component being built is deliberately not in + // here: that was the coupling this whole split is for, and with it + // included, updating one client of a two-client project killed the + // other's button for the length of a build it shares nothing with. TextButton( onClick = onUpdate, - enabled = cardState == null, + enabled = !state.busy && !projectState.busy, colors = tone.colors(), ) { Text(label) @@ -1895,13 +2058,22 @@ private fun PendingDeclaration(requested: String) { * them keeps the three in step, and there is nothing to show for the states that are not builds -- * a download or an install belongs to the APK's own controls. */ -private fun componentBuild(cardState: CardState?, component: String): ComponentBuild? = - when (cardState) { - is CardState.Pulling -> cardState.status - is CardState.Rebuilding -> cardState.status - is CardState.Preparing -> cardState.status - else -> null - }?.component(component) +private fun componentBuild( + projectState: ProjectState?, + state: ComponentState?, + component: String, +): ComponentBuild? = + // Its own first: a build this component was asked for directly is the + // one being watched, and a project-wide run is only what to fall back + // on. Both carry the same shape, and either way the slice taken is + // this component's, so a run covering several never draws one + // component's work in another's row. + (state as? ComponentState.Preparing)?.status?.component(component) + ?: when (projectState) { + is ProjectState.Pulling -> projectState.status + is ProjectState.Rebuilding -> projectState.status + else -> null + }?.component(component) /** * Says that installing this build would pair it with something older. @@ -1948,6 +2120,14 @@ private fun ComponentCard( build: ComponentBuild?, working: Boolean, busy: Boolean, + /** + * What this component has been asked to do, or why the last thing stopped. + * + * Here as well as inside [controls] because a failure has to be reported for both kinds, and + * only an APK has controls — a service action that failed would otherwise have nowhere to say + * so on the row it was pressed in. + */ + state: ComponentState?, onAction: (String, Purge) -> Unit, // Controls belonging to this component that only the caller can build // -- an APK's Update button, which needs the project's build state. @@ -2169,9 +2349,28 @@ private fun ComponentCard( // order it happened -- and putting it above meant the buttons // moved down the moment a build began, so the row somebody had // just pressed slid out from under their finger. - build?.let { - Spacer(Modifier.height(6.dp)) - ComponentBuildProgress(it) + // + // A component that has finished shows nothing at all: the + // control above has gone back to being pressable, which is + // what says the work is over, and a line reporting on it + // afterwards is one more thing to read that nobody acts on. + build + ?.takeIf { it.running } + ?.let { + Spacer(Modifier.height(6.dp)) + ComponentBuildProgress(it) + } + // The other half of "a failure is reported where it happened": + // whatever this component was last asked to do and could not, + // whether that was its build, its download, or a service + // action. In its own row rather than at the foot of the card, + // which could only ever have been the project's. + (state as? ComponentState.Error)?.let { + Text( + it.message, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.error, + ) } // Only while the old app is actually still there. The build @@ -2485,9 +2684,6 @@ private fun VariantPicker( * Only the steps that belong to the whole project. Work belonging to a component is drawn in that * component's own row by [ComponentBuildProgress], because every component builds at once and a bar * under the card could only say that something, somewhere, was happening. - * - * A build takes long enough that "is it stuck?" is a real question, so the elapsed time is shown - * rather than a bar that only moves. */ @Composable private fun BuildProgress(label: String, status: BuildStatus?) { @@ -2499,7 +2695,7 @@ private fun BuildProgress(label: String, status: BuildStatus?) { val phase = status?.phase ?: return ProgressBar() Spacer(Modifier.height(4.dp)) - Text("$label: $phase ${formatDuration(status.elapsedMs)}") + Text("$label: $phase") } /** @@ -2510,66 +2706,46 @@ private fun BuildProgress(label: String, status: BuildStatus?) { * real work, and being right most of the time is not something the person watching it can check. A * command that says nothing gets a bar that says nothing. * - * A component that has finished collapses to its duration alone. Its block stays -- a step that - * vanishes the instant it succeeds takes its own duration with it -- but the count and the last - * line it printed go, because every component builds at once and those two outlive the work they - * describe: a full bar's numbers and a frozen line of output sit there looking live next to a - * sibling that genuinely still is. What it took is the whole of what a finished component has left - * to say. The failed case keeps its error, which is an outcome rather than residue. + * A component that has finished shows nothing here at all -- the caller draws this only while + * [ComponentBuild.running]. Everything below is about work in flight: the bar, the step it is in, + * the count and the last line it printed all describe something happening now, and every one of + * them outlives the work it describes if left up. A full bar and a frozen line of output sit there + * looking live beside a sibling that genuinely is. What says the build is over is the button above + * going back to being pressable, and the card's own freshness saying the new build landed. + * + * The failure is the exception, and it is drawn by the caller rather than here: it is an outcome + * rather than residue, and it is the same message a download or a service action would leave, so it + * belongs in the one place that reports this component's failures. */ @Composable private fun ComponentBuildProgress(build: ComponentBuild) { val counted = build.progress?.takeIf { it.total > 0 } - val step = build.step - if (step != null) { - if (counted == null) { - ProgressBar() - } else { - ProgressBar(fraction = { counted.done.toFloat() / counted.total }) - } - Spacer(Modifier.height(4.dp)) + val step = build.step ?: return + if (counted == null) { + ProgressBar() + } else { + ProgressBar(fraction = { counted.done.toFloat() / counted.total }) } + Spacer(Modifier.height(4.dp)) Text( - when { - step != null -> - buildString { - append(step) - append(" ${formatDuration(build.elapsedMs)}") - if (counted != null) append(" ${counted.done}/${counted.total}") - } - // "done" for a component that fell over reads as a component - // that succeeded, and the error below it is the only thing - // saying otherwise -- so the word itself has to be able to say - // which of the two happened. - build.error != null -> "failed after ${formatDuration(build.elapsedMs)}" - else -> "done in ${formatDuration(build.elapsedMs)}" + buildString { + append(step) + if (counted != null) append(" ${counted.done}/${counted.total}") }, style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, ) - build.error?.let { + build.lastLine()?.let { Text( it, style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.error, + color = MaterialTheme.colorScheme.onSurfaceVariant, + maxLines = 2, + overflow = TextOverflow.Ellipsis, ) } - if (step != null) { - build.lastLine()?.let { - Text( - it, - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurfaceVariant, - maxLines = 2, - overflow = TextOverflow.Ellipsis, - ) - } - } } -private fun formatDuration(ms: Long): String = - if (ms < 10_000) "%.1fs".format(ms / 1000.0) else "${(ms + 500) / 1000}s" - /** * Whether the build machine is producing a new build for this card. * @@ -2579,8 +2755,15 @@ private fun formatDuration(ms: Long): String = * would take a card out of "Up to date" for the length of a download and put it straight back -- * the same jump this is here to stop. */ -private fun CardState?.isBuilding(): Boolean = - this is CardState.Pulling || this is CardState.Preparing || this is CardState.Rebuilding +private fun isBuilding( + projectState: ProjectState?, + componentStates: Map?, +): Boolean = + projectState is ProjectState.Pulling || + projectState is ProjectState.Rebuilding || + // Any one component being built is the card having something in + // flight, the same way any one component being behind is. + componentStates?.values.orEmpty().any { it is ComponentState.Preparing } private fun isUpToDate( apk: ComponentApk, diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 6a5464c..6a0b4df 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -11,7 +11,6 @@ use std::collections::{HashMap, VecDeque}; use std::path::PathBuf; use std::process::Command; use std::sync::{Arc, Mutex}; -use std::time::Instant; use serde::Serialize; @@ -85,22 +84,38 @@ struct Inner { /// [`BuildState::matches`] -- so its own copy of the components goes /// stale the moment a build records against it. built_from: HashMap, - building: bool, - /// When the current run began, for the elapsed time shown while it is - /// still going. - started: Option, + /// A pull is running. The one thing here that is the *project's* and + /// not a component's: there is one checkout, and a pull rewrites the + /// files every component builds from, so it is exclusive with all of + /// them. + /// + /// Components have no such flag between them -- whether one is being + /// built is a property of its own entry in `runs`, so building one + /// neither blocks nor says anything about the others. + pulling: bool, /// What the *project* is doing: fetching, pulling. Work belonging to /// one component is in `runs` instead, because that is where the card /// shows it -- a progress bar under the whole project could only ever /// say that something, somewhere, was happening. phase: Option, - /// One entry per component the current run has reached, in the order - /// it reached them. + /// One entry per component that has been built since this server + /// started, in the order they were first reached. + /// + /// Kept after a run rather than cleared at the start of the next one, + /// because a run is now one component's: clearing the list would + /// throw away a sibling's outcome to report on something that has + /// nothing to do with it. An entry is replaced only when *that* + /// component is claimed again. runs: Vec, - /// The failure from the last completed run, cleared when a new one - /// starts. Kept rather than logged-and-dropped because the phone is - /// where this is being driven from and usually has no access to the + /// The failure from the last pull, cleared when any new work starts. + /// Kept rather than logged-and-dropped because the phone is where + /// this is being driven from and usually has no access to the /// server's log. + /// + /// Pulls only. A build failure belongs to the component that produced + /// it (`ComponentRun::error`) -- held here it was the project's one + /// error slot, so two components building at once had one place to + /// report two outcomes. error: Option, /// Whether that failure was a pull with no fast-forward to make, /// which is the one the phone can offer a way past. Beside the @@ -108,12 +123,30 @@ struct Inner { /// button that appears only when git happened to phrase itself a /// certain way is a button nobody can rely on. unrelated_histories: bool, - /// The component whose step produced that failure. +} + +impl Inner { + /// Whether this component is being worked on right now. /// - /// Kept beside the message so the card can open the right log without - /// guessing: a component that failed to build wants its build log, - /// while every other component still wants the runtime one. - failed: Option, + /// Its own entry having an open step, which is the same thing the + /// phone reads off `ComponentStatus::step` -- so what disables a + /// component's button here and what draws its bar there cannot come + /// to disagree. + fn component_running(&self, name: &str) -> bool { + self.runs + .iter() + .any(|run| run.name == name && run.step.is_some()) + } + + /// Whether anything at all is happening for this project. + /// + /// What `building` means on the wire, and deliberately a derived + /// answer rather than a flag of its own: a flag would be a second + /// place for the truth, and the one it disagreed with would be the + /// per-component one everything else is now drawn from. + fn anything_running(&self) -> bool { + self.pulling || self.runs.iter().any(|run| run.step.is_some()) + } } /// What one component's part of the build is doing, or did. @@ -129,10 +162,6 @@ struct ComponentRun { /// What it is doing now -- building, installing, restarting -- or /// `None` once it has finished. step: Option, - started: Instant, - /// Filled in when the component finishes, so the card can keep showing - /// how long it took. - took_ms: Option, /// Steps done and steps total, when the running command reports them. progress: Option<(u64, u64)>, /// The tail of this component's output. Bounded because this is a @@ -184,7 +213,38 @@ pub struct BuildState { #[serde(rename_all = "camelCase")] pub struct BuildStatus { pub stale: bool, + /// Flattened, so `/status` answers the one flat object it always has + /// while a card can carry [`RunningBuild`] on its own. They are split + /// because `stale` is the expensive half: it walks every component's + /// directory, and `describe` is on the manifest path. + #[serde(flatten)] + pub run: RunningBuild, +} + +/// What a build is doing, with nothing in it that has to be measured off +/// disk to answer. +/// +/// This is the whole of what the phone needs to pick a build back up. It +/// is reported on the card as well as from `/status`, because the app's +/// record of a run lives only in the composition: leaving the app tears +/// down the polling loop and the card state with it, and without this the +/// list it comes back to cannot say that a build is still going. The +/// server never lost anything -- the run owns its own `Arc` +/// and outlives every request -- so the fix is for the manifest to say so +/// rather than for anything here to be re-attached to. +#[derive(Serialize, Clone)] +#[serde(rename_all = "camelCase")] +pub struct RunningBuild { + /// Anything at all is happening for this project -- a pull, or any + /// component being built. Deliberately the *project's* question, for + /// the controls that act on the whole checkout; a caller asking about + /// one component reads that component's `step` instead, because this + /// answers yes while a sibling builds and would disable a button that + /// has nothing to wait for. pub building: bool, + /// Why the last pull could not be made. Pulls only: a build failure is + /// reported against the component that produced it, since two + /// building at once have two outcomes and this is one field. pub error: Option, /// That failure was a pull the checkout has no fast-forward for, /// because it shares no history with its upstream. The phone offers @@ -193,23 +253,21 @@ pub struct BuildStatus { /// What the whole project is doing -- fetching, pulling -- absent /// between runs and while the work belongs to a component instead. pub phase: Option, - /// Milliseconds since this run started. - pub elapsed_ms: u64, - /// Each component the run has reached, in the order it reached them. - /// The card draws each of these inside that component's own row. + /// Every component built since this server started, in the order they + /// were first reached. The card draws each of these inside that + /// component's own row, and reads whether *it* is busy from its own + /// entry. pub components: Vec, } /// One component's part of a build, as the phone sees it. -#[derive(Serialize)] +#[derive(Serialize, Clone)] #[serde(rename_all = "camelCase")] pub struct ComponentStatus { pub name: String, /// What it is doing now, absent once it has finished. #[serde(skip_serializing_if = "Option::is_none")] pub step: Option, - /// How long it has been going, or took. - pub elapsed_ms: u64, /// Steps done and steps total, when the running command reports them. /// Absent for a command that says nothing, which is most of them -- /// the phone shows a bar that only spins rather than inventing a @@ -260,6 +318,12 @@ fn parse_cargo_progress(line: &str) -> Option<(u64, u64)> { /// build is doing, far short of keeping a build log in memory. const LOG_LINES: usize = 40; +/// The step a component is in while its own build command runs, named +/// once because two places have to agree on it: `claim` writes it when it +/// takes the component, and `build_component` writes it again when it +/// actually starts. +const BUILDING: &str = "building"; + /// Calls `emit` once per segment of a command's output, where a segment /// ends at a newline **or** a carriage return, and once more for anything /// left unterminated when the stream ends. @@ -393,35 +457,78 @@ impl BuildState { /// that runs here, so a project already current with its checkout /// could never record one and would report unknown for ever. /// - /// Still idempotent -- while one run is going, another does nothing, - /// whichever component either names: there is one build slot per - /// project, not one per component, so a second request while the - /// first is still running is a no-op rather than a second concurrent - /// build. The phone notices by polling `/status` and re-reads once it - /// clears. + /// Idempotent per *component*, not per project: a component already + /// being built is left alone, and every other one this selects starts + /// regardless. So two components of one project build at the same + /// time, and asking for one says nothing about the others -- which is + /// the whole point of a project being able to produce more than one + /// thing. The phone notices a component is busy by polling `/status` + /// and reading that component's own step. /// /// `component` restricts the run to one named component, or every one /// with a command for `None` -- see [`Self::trigger_if_needed`] for /// why a caller would want the former. pub fn build_now(self: &Arc, component: Option<&str>, record: RecordBuilt) { - { + let claimed = { let mut inner = self.inner.lock().unwrap(); - if inner.building { + // The one thing a component still waits behind. See + // `Inner::pulling`. + if inner.pulling { return; } - inner.building = true; - inner.error = None; - inner.failed = None; - inner.unrelated_histories = false; - inner.started = Some(Instant::now()); - inner.runs.clear(); - } + self.claim(&mut inner, component) + }; + self.start(claimed, record); + } + /// Marks each component `name` selects that has something to build + /// and is not already building, and answers where they are. + /// + /// Takes the guard rather than the lock so that a caller can do this + /// and something else in one step -- which the pull needs: releasing + /// `pulling` and claiming what it decided to build have to happen + /// together, or a phone polling in between sees a project that is + /// neither pulling nor building and reads its run as finished. + /// + /// The entry is written **here**, synchronously, rather than by the + /// build thread once it gets going. The route answers as soon as this + /// returns, so a status read in that window would otherwise find the + /// component idle -- and idle is exactly what the phone is waiting + /// for. + fn claim(&self, inner: &mut Inner, name: Option<&str>) -> Vec { + let mut claimed = Vec::new(); + for (index, component) in self.components.iter().enumerate() { + if name.is_some_and(|name| component.name() != name) { + continue; + } + if component.build().is_empty() { + continue; + } + if inner.component_running(component.name()) { + continue; + } + // Whatever the last run left against this component goes now, + // so nothing from it can be read as this run's outcome -- an + // old error in particular, which the phone treats as a reason + // not to download. + reset_run(inner, component.name()); + claimed.push(index); + } + if !claimed.is_empty() { + // The pull this describes is the one being built on top of. + inner.error = None; + inner.unrelated_histories = false; + } + claimed + } + + /// Runs what [`Self::claim`] claimed, off the request's thread. + fn start(self: &Arc, claimed: Vec, record: RecordBuilt) { + if claimed.is_empty() { + return; + } let this = Arc::clone(self); - let component = component.map(str::to_string); - tokio::task::spawn_blocking(move || { - this.run_build(component.as_deref(), &record); - }); + tokio::task::spawn_blocking(move || this.run_claimed(claimed, &record)); } /// Whether any component `name` selects is behind. Whole project for @@ -531,8 +638,7 @@ impl BuildState { /// Fetches, fast-forwards if there is anything to take, and builds -- /// the Pull button, which acts on the build machine rather than the - /// phone. Idempotent in the same way as [`Self::trigger_if_needed`]: - /// while one is running, another does nothing. + /// phone. While one is running, another does nothing. /// /// Building happens when the pull actually moved the branch, or when /// the configured staleness rule says the output is behind anyway; a @@ -558,6 +664,12 @@ impl BuildState { /// reported that the two are unrelated (`git::PullError`). It is a /// parameter rather than something decided here because it is a /// person's answer to that report, not a state of the repository. + /// + /// Unlike a build, this is exclusive with everything the project is + /// doing: it rewrites the one checkout every component is built from, + /// so it waits for any component still building and blocks any that + /// would start. That is the project's own coupling rather than the + /// components', and it is the only one left here. pub fn pull_and_build( self: &Arc, force: bool, @@ -566,15 +678,12 @@ impl BuildState { ) { { let mut inner = self.inner.lock().unwrap(); - if inner.building { + if inner.anything_running() { return; } - inner.building = true; + inner.pulling = true; inner.error = None; - inner.failed = None; inner.unrelated_histories = false; - inner.started = Some(Instant::now()); - inner.runs.clear(); } let this = Arc::clone(self); @@ -587,16 +696,27 @@ impl BuildState { } Ok(pulled) => { // Nothing configured to build, or nothing allowed to: - // the pull was the whole job, and reporting success is - // all that is left. + // the pull was the whole job. // `may_build()` is called here, with the pulled // declaration on disk, for the reason in the doc - // comment above. - if !may_build() || !this.has_command() || !(pulled || this.is_stale(None)) { - this.finish(None, None); - } else { - this.run_build(None, &record); - } + // comment above. Both it and `is_stale` take the lock + // themselves, so they are asked before it is held. + let build = + may_build() && this.has_command() && (pulled || this.is_stale(None)); + // Handing the run over in one step, so nothing can + // observe the moment between the pull ending and the + // builds it decided on starting -- see `claim`. + let claimed = { + let mut inner = this.inner.lock().unwrap(); + let claimed = match build { + true => this.claim(&mut inner, None), + false => Vec::new(), + }; + inner.pulling = false; + inner.phase = None; + claimed + }; + this.run_claimed(claimed, &record); } } }); @@ -624,69 +744,64 @@ impl BuildState { Ok(true) } - /// Builds every component `name` selects, at once. + /// Builds the claimed components, at once, and reports each against + /// itself. /// /// Every one of them together rather than in turn -- they are /// independent, a Rust build and a Gradle build share nothing but the /// machine, and measured on this one, running them together takes - /// about three quarters of the time running them in turn does. The - /// saving only appears when more than one has work to do, which is - /// the case a pull produces; `name` is how a caller that wants only - /// one opts out of paying for the others (see - /// [`Self::trigger_if_needed`]) -- `None` still means all of them. + /// about three quarters of the time running them in turn does. /// /// What running them together costs is that a failure no longer stops /// the others: they are already running by the time it happens, so - /// stopping them would mean killing work that is probably fine, and - /// the first failure in declaration order is the one reported, which - /// is what a walk in that order would have said. - /// - /// Each component's name is the phase name, so the card says which one - /// is being worked on without needing anything new to carry it. - fn run_build(self: &Arc, name: Option<&str>, record: &RecordBuilt) { + /// stopping them would mean killing work that is probably fine. There + /// is no "the run failed" left to report either way -- an outcome + /// belongs to the component that produced it, and the phone reads it + /// off that component's own entry. + fn run_claimed(self: &Arc, claimed: Vec, record: &RecordBuilt) { let mut running = Vec::new(); - for (index, component) in self.components.iter().enumerate() { - if name.is_some_and(|name| component.name() != name) { - continue; - } - if component.build().is_empty() { - continue; - } + for index in claimed { let this = Arc::clone(self); let record = Arc::clone(record); running.push(( - component.name().to_string(), + self.components[index].name().to_string(), std::thread::spawn(move || this.build_indexed(index, &record)), )); } - let mut error = None; - // Which component's step ended the walk, so the card can open that - // component's *build* log rather than its runtime one. - let mut failed = None; + // Whether anything in *this* run fell over, which is only asked + // about the restart below. It is not reported anywhere: the + // component that failed has already recorded its own message. + let mut failed = false; // Set by the component that is this process; acted on once every - // other component has finished and the whole run is reported. See - // `is_self`. + // other component in the run has finished. See `is_self`. let mut restart_self = false; for (name, handle) in running { let outcome = match handle.join() { - Ok(outcome) => outcome, // A panic in a build thread is this server's bug, not the // project's, but the card still has to say something -- // silence would read as a build that simply did nothing. - Err(_) => Err(format!("building {name} panicked -- see this server's log")), + // Recorded here because the panic is what stopped + // `build_indexed` from doing it, and a component left with + // an open step never stops looking busy. + Err(_) => { + let message = format!("building {name} panicked -- see this server's log"); + self.finish_component(&name, Some(message.clone())); + Err(message) + } + Ok(outcome) => outcome, }; match outcome { Ok(is_self) => restart_self |= is_self, - Err(message) if error.is_none() => { - error = Some(message); - failed = Some(name); - } - Err(_) => {} + Err(_) => failed = true, } } - let mut restart = error.is_none() && restart_self; + // Not restarting on a failure is about *this process*, not about + // the components: an exec drops the connection the phone is + // reading the failure over, and it would lose the report it is + // waiting for. + let mut restart = !failed && restart_self; // A build with nothing to do leaves the binary alone, and exec-ing // into the same file would drop the phone's connection to deliver // the build it already had. @@ -696,10 +811,10 @@ impl BuildState { ); restart = false; } - self.finish(error, failed); if restart { - // Answered and finished first, so whatever happens next cannot - // take the report away from whoever asked for it. + // Every component has closed its own entry by now, so whatever + // happens next cannot take the report away from whoever asked + // for it. crate::restart::deferred(self.handover(), Arc::clone(&self.shared.downloads)); } } @@ -733,7 +848,7 @@ impl BuildState { component: &Component, record: &RecordBuilt, ) -> Result { - self.begin_component(component.name(), "building"); + self.begin_component(component.name(), BUILDING); if let Err(message) = self.run_streaming(component) { tracing::error!("{} failed: {message}", component.name()); return Err(message); @@ -985,16 +1100,18 @@ impl BuildState { Ok(process) } - /// Marks the start of a step, closing the previous one with its - /// duration so the phone can show where the time went. /// Marks what the *project* is doing. Only fetching and pulling: the /// rest belongs to a component. fn begin_project(&self, name: &str) { self.inner.lock().unwrap().phase = Some(name.to_string()); } - /// Marks one component as doing `step`, starting its entry if this is - /// the first thing it has done this run. + /// Marks one component as doing `step`. + /// + /// Its entry already exists -- `claim` writes it before the build + /// thread starts, so that nothing can read the component as idle in + /// between -- but it is created here if it somehow does not, since a + /// step nobody can see is worse than a duplicated one. fn begin_component(&self, component: &str, step: &str) { let mut inner = self.inner.lock().unwrap(); // The project-wide phase is over once a component is working; @@ -1006,51 +1123,36 @@ impl BuildState { // A count belongs to the step that printed it. run.progress = None; } - None => inner.runs.push(ComponentRun { - name: component.to_string(), - step: Some(step.to_string()), - started: Instant::now(), - took_ms: None, - progress: None, - log: VecDeque::new(), - error: None, - }), + None => reset_run(&mut inner, component), } } /// Marks one component as done, with why it stopped if it failed. + /// + /// The end of a run, now that a run is one component's: there is no + /// project-wide "finished" left to say, and the phone reads this + /// component's closed step as the answer for this component alone. fn finish_component(&self, component: &str, error: Option) { let mut inner = self.inner.lock().unwrap(); if let Some(run) = inner.runs.iter_mut().find(|run| run.name == component) { - run.took_ms = Some(run.started.elapsed().as_millis() as u64); run.step = None; run.error = error; } } - /// Closes the current step and ends the run. - fn finish(&self, error: Option, failed: Option) { - let mut inner = self.inner.lock().unwrap(); - inner.phase = None; - inner.building = false; - // Cleared so the elapsed time belongs to a run in progress rather - // than counting up forever on an idle server. - inner.started = None; - inner.error = error; - inner.failed = failed; - } - - /// Finishes a run that never got past its pull. + /// Ends a pull that could not be made. /// - /// Separate from [`Self::finish`] only because a pull failure carries - /// the one thing a build failure cannot: whether abandoning this - /// checkout's own history would clear it. Cleared where its siblings - /// are, at the start of every run, so this is the only thing that can + /// The project's own failure rather than a component's, and it + /// carries the one thing a build failure cannot: whether abandoning + /// this checkout's own history would clear it. Cleared at the start of + /// anything that runs afterwards, so a pull is the only thing that can /// ever make it true. fn fail_pull(&self, error: crate::git::PullError) { - self.inner.lock().unwrap().unrelated_histories = error.unrelated_histories; - // No component: a pull fails before the walk starts. - self.finish(Some(error.message), None); + let mut inner = self.inner.lock().unwrap(); + inner.unrelated_histories = error.unrelated_histories; + inner.error = Some(error.message); + inner.phase = None; + inner.pulling = false; } /// Records how far the running command says it has got. Replaces the @@ -1086,14 +1188,32 @@ impl BuildState { crate::config::has_command(&self.components) } - /// Whether the last completed build stopped at this component. + /// Whether this component's last build stopped at it. /// /// The card asks so it can open that component's build log first. A - /// build that has not failed leaves every component answering false, - /// which is what makes the runtime log the ordinary default. + /// component that has not failed answers false, which is what makes + /// the runtime log the ordinary default -- and it is asked of the + /// component's own entry, so one component's failure cannot send + /// another's card to the wrong tab. pub fn build_failed(&self, component: &str) -> bool { + self.inner + .lock() + .unwrap() + .runs + .iter() + .any(|run| run.name == component && run.error.is_some()) + } + + /// What this project is doing, or `None` when nothing is. + /// + /// Deliberately not [`Self::status`], which also answers `stale` and + /// so walks every component's directory. This is reached from + /// `describe`, which builds every card on `/manifest` -- fetched on + /// every open, resume and Refresh -- so it is a lock and some clones + /// and nothing else. + pub fn running(&self) -> Option { let inner = self.inner.lock().unwrap(); - inner.error.is_some() && inner.failed.as_deref() == Some(component) + inner.anything_running().then(|| Self::snapshot(&inner)) } pub fn status(&self) -> BuildStatus { @@ -1102,24 +1222,24 @@ impl BuildState { let inner = self.inner.lock().unwrap(); BuildStatus { stale, - building: inner.building, + run: Self::snapshot(&inner), + } + } + + /// The one place the run is read out of the lock, so `/status` and a + /// card cannot come to describe the same build differently. + fn snapshot(inner: &Inner) -> RunningBuild { + RunningBuild { + building: inner.anything_running(), error: inner.error.clone(), unrelated_histories: inner.unrelated_histories, phase: inner.phase.clone(), - elapsed_ms: inner - .started - .map(|started| started.elapsed().as_millis() as u64) - .unwrap_or(0), components: inner .runs .iter() .map(|run| ComponentStatus { name: run.name.clone(), step: run.step.clone(), - // Still going, or however long it took. - elapsed_ms: run - .took_ms - .unwrap_or_else(|| run.started.elapsed().as_millis() as u64), progress: run .progress .filter(|(_, total)| *total > 0) @@ -1135,6 +1255,29 @@ impl BuildState { } } +/// Starts this component's entry over, replacing whatever the last build +/// of it left behind. +/// +/// The step is set here rather than by the thread that will do the work, +/// so there is no window in which a claimed component reads as idle. The +/// word is the first thing every build does anyway (`build_component`), +/// so nothing has to say it twice. +fn reset_run(inner: &mut Inner, component: &str) { + let fresh = ComponentRun { + name: component.to_string(), + step: Some(BUILDING.to_string()), + progress: None, + log: VecDeque::new(), + error: None, + }; + match inner.runs.iter_mut().find(|run| run.name == component) { + // In place, so the order the phone receives components in stays + // the order it first saw them. + Some(run) => *run = fresh, + None => inner.runs.push(fresh), + } +} + fn truncate_tail(s: &str, max_chars: usize) -> String { let char_count = s.chars().count(); if char_count <= max_chars { @@ -1298,7 +1441,7 @@ mod tests { move || crate::config::project_config(&project).matches_accepted(&accepted, None), Arc::new(|_: &str, _: String| {}), ); - while state.status().building { + while state.status().run.building { tokio::time::sleep(std::time::Duration::from_millis(10)).await; } @@ -1347,7 +1490,7 @@ mod tests { state.build_now(Some("app"), Arc::new(|_: &str, _: String| {})); let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); - while state.status().building { + while state.status().run.building { assert!( std::time::Instant::now() < deadline, "build did not finish in time" @@ -1367,6 +1510,7 @@ mod tests { let status = state.status(); assert_eq!( status + .run .components .iter() .map(|c| c.name.as_str()) @@ -1376,6 +1520,154 @@ mod tests { ); } + /// Building one component must neither block another's build nor make + /// it look busy. + /// + /// The complaint this decoupling is for: with one build slot per + /// project, pressing Update on one client of a two-client project + /// left the other's request a silent no-op and its buttons disabled + /// for the length of a build it had nothing to do with. Asserted from + /// *inside* the slow component's build, because "both finished + /// eventually" is equally true of running them one after the other -- + /// what is being tested is that the second one does not wait. + #[tokio::test] + async fn one_component_building_does_not_hold_up_another() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + for name in ["slow", "quick"] { + std::fs::create_dir_all(root.join(name)).expect("mkdir"); + } + // A script rather than `sh -c '...'`: a command is split on + // whitespace and never goes through a shell, so the quotes would + // be four literal arguments and the "slow" build would fail + // instantly -- which reads exactly like the two running in turn. + let script = root.join("slow.sh"); + std::fs::write(&script, "#!/bin/sh\nsleep 0.6\ntouch slow-built\n").expect("write"); + std::fs::set_permissions(&script, std::os::unix::fs::PermissionsExt::from_mode(0o755)) + .expect("chmod"); + + let components = ["slow", "quick"] + .map(|name| Component::Apk { + name: name.to_string(), + build: crate::config::Command::from_line(match name { + "slow" => "./slow.sh", + _ => "touch quick-built", + }), + cwd: Some(PathBuf::from(name)), + stale_when: None, + strip: false, + package: None, + built_from: None, + }) + .to_vec(); + let state = state_for(root, components); + + state.build_now(Some("slow"), Arc::new(|_: &str, _: String| {})); + state.build_now(Some("quick"), Arc::new(|_: &str, _: String| {})); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + loop { + assert!( + std::time::Instant::now() < deadline, + "quick never finished, so it was waiting on slow", + ); + let status = state.status().run; + let quick = status.components.iter().find(|c| c.name == "quick"); + if quick.is_some_and(|quick| quick.step.is_none()) { + assert!( + status + .components + .iter() + .any(|c| c.name == "slow" && c.step.is_some()), + "slow finished first, so this proves nothing about waiting", + ); + break; + } + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + assert!(root.join("quick/quick-built").exists(), "quick really ran"); + assert!( + !root.join("slow/slow-built").exists(), + "slow is still going, which is the point", + ); + + while state.status().run.building { + assert!( + std::time::Instant::now() < deadline, + "slow did not finish in time" + ); + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + assert!( + root.join("slow/slow-built").exists(), + "and the one that was still going finished on its own", + ); + } + + /// A component's failure is its own, and does not become the + /// project's. + /// + /// With one error slot per project, a failed build reported itself on + /// every component's card and turned every Update button into Retry. + #[tokio::test] + async fn a_failure_is_reported_against_the_component_that_produced_it() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + for name in ["broken", "fine"] { + std::fs::create_dir_all(root.join(name)).expect("mkdir"); + } + let components = ["broken", "fine"] + .map(|name| Component::Apk { + name: name.to_string(), + build: crate::config::Command::from_line(match name { + "broken" => "false", + _ => "true", + }), + cwd: Some(PathBuf::from(name)), + stale_when: None, + strip: false, + package: None, + built_from: None, + }) + .to_vec(); + let state = state_for(root, components); + + state.build_now(None, Arc::new(|_: &str, _: String| {})); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while state.status().run.building { + assert!( + std::time::Instant::now() < deadline, + "build did not finish in time" + ); + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + + let status = state.status().run; + assert!( + status.error.is_none(), + "the project's error slot is for pulls, not for a component's build", + ); + let component = |name: &str| { + status + .components + .iter() + .find(|c| c.name == name) + .expect("an entry per component") + .error + .clone() + }; + assert!(component("broken").is_some(), "the one that failed says so"); + assert!( + component("fine").is_none(), + "and the one that did not is left alone", + ); + assert!(state.build_failed("broken")); + assert!( + !state.build_failed("fine"), + "so its card opens the runtime log, not a build log it has no failure in", + ); + } + /// A sibling that has already been built must not mask that this one /// never has. /// @@ -1431,7 +1723,7 @@ mod tests { state.trigger_if_needed(Some("b"), Arc::new(|_: &str, _: String| {})); let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); - while state.status().building { + while state.status().run.building { assert!( std::time::Instant::now() < deadline, "build did not finish in time" diff --git a/server/src/routes.rs b/server/src/routes.rs index 38f993b..bebf039 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -493,6 +493,23 @@ struct ManifestApp { /// one flag for the list, so a card says whether *it* is the one still /// being worked out. check_pending: bool, + /// The build running for this project right now, if one is, in the + /// same shape `/status` answers. + /// + /// Here so that a build survives leaving the app. The run itself never + /// stops -- it owns its own `Arc` and outlives the request + /// that started it -- but the phone's record of it lives only in the + /// composition, so backgrounding tears down the polling loop and the + /// card state together. Without this the list it comes back to cannot + /// say a build is still going, and the card reads as one that was + /// killed: it offers Update again, and pressing it does nothing, + /// because there is already a run in this project's one build slot. + /// + /// Read with [`crate::build_state::BuildState::running`] rather than + /// `status`, which also answers `stale` and walks every component's + /// directory to do it -- this is on the manifest path. + #[serde(skip_serializing_if = "Option::is_none")] + build: Option, /// What this project produces, in the order it is built. One entry is /// the ordinary case and the card shows it inline; more than one is /// what the phone draws as a nested list. @@ -656,6 +673,7 @@ async fn describe(state: &Arc, entry: &AppEntry) -> Result