diff --git a/AGENTS.md b/AGENTS.md index 8919e9d..2859793 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -335,6 +335,41 @@ mutable at runtime from the phone. on *that* answer, which the card's own refresh control and a freshly added app both use. Waiting on the whole list to settle would make one card's refresh sit behind another card's. + Arriving and resuming are **one** event, not two: `LifecycleResumeEffect` + runs when the screen first reaches RESUMED, so there is no + `LaunchedEffect(Unit)` beside it and one thing decides when the list is + read. It refreshes from *any* state including a failed one -- guarded on + "loaded", as it used to be, a card that had gone red stayed red until + somebody found the Refresh button, which is the opposite of what + returning to an app should do. `loadingList` is what stops the first + composition and the first resume stacking two reads, and it is claimed + *before* the coroutine launches, because both run in the same frame and + a flag set inside the coroutine is set too late to be a guard. + +- **A failure that lands while nobody is looking is not shown.** Work + started before the app went away keeps running -- deliberately, since a + download that finishes in the background is a download that worked -- and + when the device sleeps or the link drops it fails. Reported, that meant + coming back an hour later to a five-second read timeout that said nothing + about the server and that there was nothing left to do about. So every + catch in `UpdaterScreen.kt` goes through `failure(e)`, which answers null + while the screen is not resumed, and **null means clear, never leave** at + every site -- a dropped failure that left the card alone would leave a + spinner up for an operation that has already stopped. `setProject` and + `setComponent` both take a nullable state so the answer can be handed + straight over. Iris asked for this on 2026-09-01: "I got a socket timeout + by leaving the app for too long. Make sure not to show that if the app + just gets unloaded." + The other half of the same complaint is the read on the way back *in*: + the link may have been asleep as long as the app was, and the first + request across one still coming back times out at the five seconds every + request gets. So the resume's read alone retries once (`afterAGap`), + which costs nothing when the server really is down -- a refused + connection comes back at once rather than waiting out a timeout. That is + a retry, not a guess: nothing is displayed that was not measured. + Dropping a failure is only safe *because* the resume re-reads from any + state; the two changes hold each other up, and undoing either alone + leaves the list stuck on a spinner or stuck on a stale error. - **The Add screen is drawn over the list, not in place of it.** Swapped out, the list is composed again from nothing on the way back -- and 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 4631946..cd3caf5 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -140,6 +140,14 @@ private const val CHECK_POLL_INTERVAL_MS = 700L // second; these cover that with room to spare without making a genuine // outage take long to report. private const val REFRESH_ATTEMPTS_AFTER_PULL = 4 + +/** + * How long to wait before the one retry a resume gets. + * + * Short, because this is not waiting for a server to come up -- it is giving a link that woke with + * the screen a moment to finish doing so. + */ +private const val WAKE_RETRY_MS = 600L private const val RESTART_WAIT_MS = 1000L private sealed class ManifestState { @@ -489,6 +497,42 @@ private fun AppListScreen( // that is busy is the one that shows it. var serviceBusy by remember { mutableStateOf>(emptyMap()) } + // Whether this screen is actually in front of somebody. Set by the + // resume effect further down, and read only by `failure` below. + var foreground by remember { mutableStateOf(true) } + + /** + * What to show for a failed request, or null when there is nobody to show it to. + * + * Work started before the app went away keeps running — a poll loop, a download — and when the + * device sleeps or the link drops it fails. Reporting that means coming back an hour later is + * greeted by a five-second read timeout that says nothing about the server and that there is + * nothing left to do about, because the thing it describes is over. So a failure that lands + * while nothing is on screen is dropped, and the re-read on the way back in is what produces + * the truth instead. + * + * Null means *clear*, never *leave*, at every call site: a dropped failure that left the card + * as it was would leave a spinner up for an operation that has already stopped. + * + * A failure of something somebody pressed is never dropped, because pressing it is what put + * them in front of the screen. + */ + fun failure(e: DownloadServerException): String? = (e.message ?: "Failed").takeIf { foreground } + + /** + * Puts one project's state, or takes it away for null. + * + * The pair of [setComponent], so the two levels are written the same way and a caller with a + * message that may or may not be worth showing can hand the answer straight over. + */ + fun setProject(key: String, state: ProjectState?) { + projectStates = + when (state) { + null -> projectStates - key + else -> projectStates + (key to state) + } + } + /** * Puts one component's state, or takes it away for null. * @@ -549,34 +593,70 @@ private fun AppListScreen( // of the gesture is that the list stays where it is. var pulling by remember { mutableStateOf(false) } + // Whether a whole-list read is already running, so the resume below + // cannot start a second one beside the one that is already going. + var loadingList by remember { mutableStateOf(false) } + + /** + * Reads the whole list into [manifestState], however it was asked for. + * + * A failure that lands while nothing is on screen leaves the list on the spinner rather than + * replacing it with a timeout nobody is there to read — see [failure]. That is only safe + * because the resume effect re-reads from *any* state, spinner included, so the way back into + * the app is what resolves it. + */ + // Clears the flag rather than setting it: the callers set it before + // they launch, because two of them can run in the same frame and both + // would pass the guard if the first one only claimed it once its + // coroutine got going. + // + // [afterAGap] is the resume's, and nothing else passes it. The link may + // have been asleep as long as the app was, and the first request across + // one that is still coming back times out at the five seconds every + // request here gets -- which is the server being reported unreachable + // for no reason except that somebody had the app closed. One more try + // is the difference between saying that and giving it a second. It + // costs nothing when the server really is down, because a refused + // connection comes back at once rather than waiting out the timeout. + suspend fun loadInto(afterAGap: Boolean = false) { + val attempts = if (afterAGap) 2 else 1 + try { + repeat(attempts) { attempt -> + try { + manifestState = load() + return + } catch (e: DownloadServerException) { + if (attempt == attempts - 1) { + failure(e)?.let { manifestState = ManifestState.Error(it) } + return + } + delay(WAKE_RETRY_MS) + } + } + } finally { + loadingList = false + } + } + fun refreshByPull() { - if (pulling) return + if (pulling || loadingList) return pulling = true + loadingList = true projectStates = emptyMap() componentStates = emptyMap() scope.launch { - manifestState = - try { - load() - } catch (e: DownloadServerException) { - ManifestState.Error(e.message ?: "Unknown error") - } + loadInto() pulling = false } } - fun refresh() { + fun refresh(afterAGap: Boolean = false) { + if (loadingList) return + loadingList = true manifestState = ManifestState.Loading projectStates = emptyMap() componentStates = emptyMap() - scope.launch { - manifestState = - try { - load() - } catch (e: DownloadServerException) { - ManifestState.Error(e.message ?: "Unknown error") - } - } + scope.launch { loadInto(afterAGap) } } /** @@ -664,8 +744,7 @@ private fun AppListScreen( try { awaitCheck(entry.key) } catch (e: DownloadServerException) { - projectStates = - projectStates + (entry.key to ProjectState.Error(e.message ?: "Failed")) + setProject(entry.key, failure(e)?.let(ProjectState::Error)) } } } @@ -692,11 +771,11 @@ private fun AppListScreen( start: suspend () -> BuildStatus, progress: (BuildStatus?) -> ProjectState, ) { - projectStates = projectStates + (entry.key to progress(null)) + setProject(entry.key, progress(null)) try { var status = withContext(Dispatchers.IO) { start() } while (status.building) { - projectStates = projectStates + (entry.key to progress(status)) + setProject(entry.key, progress(status)) delay(BUILD_POLL_INTERVAL_MS) status = withContext(Dispatchers.IO) { buildStatus(entry.key) } } @@ -706,9 +785,9 @@ private fun AppListScreen( // 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) { - projectStates = projectStates + (entry.key to ProjectState.Error(failure)) + val pullFailure = status.error + if (pullFailure != null) { + setProject(entry.key, ProjectState.Error(pullFailure)) // 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. @@ -745,7 +824,7 @@ private fun AppListScreen( // 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 + setProject(entry.key, null) // 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, @@ -756,15 +835,15 @@ private fun AppListScreen( return } catch (e: DownloadServerException) { if (attempt == REFRESH_ATTEMPTS_AFTER_PULL - 1) { - projectStates = projectStates - entry.key - manifestState = ManifestState.Error(e.message ?: "Unknown error") + setProject(entry.key, null) + failure(e)?.let { manifestState = ManifestState.Error(it) } } else { delay(RESTART_WAIT_MS) } } } } catch (e: DownloadServerException) { - projectStates = projectStates + (entry.key to ProjectState.Error(e.message ?: "Failed")) + setProject(entry.key, failure(e)?.let(ProjectState::Error)) } } @@ -835,11 +914,7 @@ private fun AppListScreen( return@launch } } catch (e: DownloadServerException) { - setComponent( - entry.key, - component, - ComponentState.Error(e.message ?: "Couldn't prepare phone build"), - ) + setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) return@launch } } @@ -860,11 +935,7 @@ private fun AppListScreen( } } } catch (e: DownloadServerException) { - setComponent( - entry.key, - component, - ComponentState.Error(e.message ?: "Download failed"), - ) + setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) return@launch } setComponent(entry.key, component, null) @@ -881,11 +952,10 @@ 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. - projectStates = projectStates - entry.key + setProject(entry.key, null) if (removes) dropEntry(entry.key) else applyOne(entry.key) } catch (e: DownloadServerException) { - projectStates = - projectStates + (entry.key to ProjectState.Error(e.message ?: "Failed")) + setProject(entry.key, failure(e)?.let(ProjectState::Error)) } } } @@ -926,7 +996,7 @@ private fun AppListScreen( ) applyOne(entry.key) } catch (e: DownloadServerException) { - setComponent(entry.key, component, ComponentState.Error(e.message ?: "Failed")) + setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) } finally { serviceBusy = serviceBusy - entry.key } @@ -961,7 +1031,10 @@ private fun AppListScreen( } } - LaunchedEffect(Unit) { refresh() } + // The first load is the resume effect's too, rather than a + // LaunchedEffect(Unit) beside it: `LifecycleResumeEffect` runs when the + // screen first reaches RESUMED, so arriving and returning are the same + // event and there is one thing that decides when the list is read. // An app added on the Add screen is fetched on its own and appended. // Reloading the list instead would put every other card back through @@ -1016,11 +1089,25 @@ private fun AppListScreen( // true now rather than whenever it was last opened. A checkout already // being asked about is not asked again (RemoteChecks::refresh), so a // burst of resumes doesn't pile up round trips. + // + // From *any* state, not only a loaded one, and that is load-bearing + // twice over. It is what makes dropping a background failure safe: + // that leaves the list on its spinner, and this is the only thing that + // will take it off. And a state that failed is the one most worth + // retrying on the way back in -- guarded on "loaded", a card that had + // gone red stayed red until somebody found the Refresh button, which + // is the opposite of what returning to an app should do. + // + // `refresh` declines when a read is already running, so this and the + // first-composition run below cannot stack two. LifecycleResumeEffect(Unit) { - if (manifestState is ManifestState.Loaded) { - refresh() - } - onPauseOrDispose {} + foreground = true + refresh(afterAGap = true) + // Not "the app is gone" -- the coroutines started above keep + // running, deliberately, because a download that finishes while + // somebody is in another app is a download that worked. This only + // says there is nobody to report a failure to. + onPauseOrDispose { foreground = false } } // PackageManager reports a completed install as soon as it happens,