feat: follow host-owned task runs from schema actions #4
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/task-runs"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes the mobile half of the task/action overhaul (core#21).
Why
Core made long work a run the host owns: an action's route starts it and answers
{ runId }instead of doing the work inside the request. The app had no notion of that, and the failure mode was silent in the worst possible way.With no
progressTopic,runActionclearedrunningActionand refetched on any 2xx. So against a migrated module, the app reported success the instant the run was created — before any work ran. A run that then failed showed nothing at all.This was blocking module migrations: every schema-visible action (
module_systemd's unit verbs,module_ipmi's refresh,module_updates' apt runs) had to keep answering a synchronous verdict purely because Android could not follow a run.What
The response decides the protocol; no schema field selects it. A body carrying a numeric
runIdmeans the work is not over, so the app followscore:task:<runId>and lets the run's terminal status end the action. Both write paths do this —runActionandsubmitForm.That topic is retained, which makes this better than the stream it replaces rather than merely equivalent:
progressTopicdone/errorsinceLiveClientlearns retained topics to make that possible: thesnapframe, the per-topictseqit carries, and theepochscoping that sequence to one incarnation of the topic. Resubscribing sendssince/epochso the server replays the gap or re-snapshots.Resyncis now emitted only for topics that cannot replay — sending it for a retained one would tell the caller to go re-read REST and undo the point of retaining.Progress entities are the same shape a run reports, so
ProgressFeedandUpdateProgressCardrender both paths unchanged.Two gaps closed while wiring it up:
phaseis now carried forward on merge and read: only thedownloadhalf is summed, because compressed and uncompressed byte counts must never be added together.ProgressKindgained the kinds core names (pull,step) plus anOTHERdefault. The wire format allows any string there, and an unknown kind used to take the whole event down with it.progressTopicstays. It is deprecated, not gone, and servers and modules that have not migrated still speak it.The channel is a fast path, not the only path
Following a run must not be the only thing that can release the screen, because the channel can fail in ways the operator cannot see:
Upgrade, blocked port, server too old for the topic): a watchdog gives the subscription 10s to deliver its snapshot — a retained topic answers on subscribe, so silence that long is a dead channel, not a quiet run — and then follows the run overGET /api/core/tasks/:idevery 3s instead.gap, which withholdssincefrom the nextsubframe (a subscription with a hole must not promise it has everything throughtseq), and re-subscribes immediately rather than waiting for a reconnect that may never come. If that snapshot is dropped too, the first delivered event after it retries. The resume state now lives inSubState, testable without a socket.finishedAtMsends the follow whateverRunStatus.UNKNOWNimplies.capabilities.cancelis decoded, so "Cancel run" appears when core says this viewer may, and "Stop following" always does — it releases the screen and leaves the run running host-side, to be picked up from the topic again later.Only the actions that actually conflict are disabled while a run is followed:
pathactions share the feed and usually the server-side lock, navigation entries andopendownloads share neither. Downloads now have their own busy state.Deliberately not handled
entities/lineswith their names inwithheld. The status still lands, so the button still finishes; the rows are simply absent.stepandcheckpointevents. Of the five event types on the topic,status,progressandlogare rendered;stepandcheckpointare dropped.RunStepis decoded (it arrives inside the snapshot) but nothing shows it yet — for an apt run those steps are the only structure between log lines, so this is worth a follow-up.retry. Decoded as a capability, not offered.successPagewhen the submit started a run. Navigating away cancels the follow, which would put back the silent success this PR exists to remove, so the run is watched on the page that started it.progressandrunningActionare single-valued; the model rejects a second action while one runs rather than letting the two share a feed.followRunis deliberately not passed throughRefreshPolicy.gate— its own doc says not to gate user-initiated flows like action progress, and the existingprogressTopicpath does not either.Verification
./gradlew assembleDebugand./gradlew testDebugUnitTestgreen (67 tests).New tests cover
SubState's whole state machine (tseq adoption on snapshot, advance only on delivered events, gap, the re-subscribe and its retry,since/epochon the frame),ProgressFeed.of(order, last-wins, the 500-line tail) and what ends a run (nullsnapshot, unknown status,finishedAtMs, failure wording).followRun's coroutine wiring itself is not unit-tested — the ViewModel needs anApplicationand the container, and this module has neither Robolectric norcoroutines-test.Not yet exercised against a live server — the modules that answer
{ runId }from a schema action are the companion PR (module_updates), andmodule_systemd/module_ipmideliberately still answer a synchronous verdict on main until this lands.🤖 Generated with Claude Code
Core's task/action overhaul (core#21) made long work a run the host owns: the action's route starts it and answers { runId } instead of doing the work inside the request. The app had no notion of that, and the failure was silent in the worst way — with no progressTopic, runAction cleared runningAction and refetched on any 2xx, so a migrated action reported success the instant the run was CREATED. A run that then failed showed nothing at all. The response decides which protocol applies; no schema field selects it. A body carrying a numeric runId means the work is not over, so the app follows core:task:<runId> and lets the run's terminal status end the action. That topic is retained, which is what makes this better than the stream it replaces rather than merely equivalent: - The snapshot IS the whole run so far, so subscribing after the POST has no race, and following a run that already finished works exactly like following one that just started. - The end is the run reaching a terminal state, not a log line beginning with "done" — module output stopped being load-bearing. - Backgrounding the app and returning resumes from `since`; the old stream lost whatever was pushed while it was away. LiveClient learns retained topics to make that possible: a `snap` frame, the per-topic `tseq` it carries, and the `epoch` that scopes it to one incarnation of the topic. Resubscribing sends `since`/`epoch` so the server replays the gap or re-snapshots. Resync is now emitted only for topics that cannot replay — sending it for a retained one would tell the caller to go re-read REST and undo the point of retaining. Progress entities are the same shape a run reports, so ProgressFeed and UpdateProgressCard render both paths unchanged. Two gaps closed while wiring it: `phase` is carried forward on merge (counts from a download and an extract must never be added together), and ProgressKind gained the kinds core names plus an OTHER default, because the wire format allows any string and an unknown kind used to take the whole event down with it. progressTopic stays. It is deprecated, not gone, and modules and servers that have not migrated still speak it. Fields a viewer is not entitled to see arrive empty with their names in `withheld` — the status still lands, so the button still finishes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Reviewed against core's actual contracts (
packages/sdk/tasks.ts,packages/server/src/events/live.ts,packages/server/src/tasks/routes.ts) and the existing web reference implementation inpackages/modules/docker/frontend/, rather than against the description.The retained-topic protocol work is right: the
snap/tseq/epoch/sincehandling matcheslive.tsframe-for-frame, theResyncsuppression for retained topics is correct, and theis LiveEvent.Snapshot -> Unitbranch instartLogFollowsis a valid assertion — providers are never retained. What follows is what does not hold.Blockers
1.
PULLis not a layer — the aggregate bar is now wrongUpdateProgress.kt:169kind: "pull"is docker's synthetic rollup, not a layer —compose_progress.ts:644emits it withkey: "pull/summary",id: "pull", and its numbers live in apullfield this model does not carry. It has nototaland nofraction. Its own doc says so: "'pull' is not a compose entity at all: it is the synthetic rollup this module derives".Two consequences:
pullFraction(line 200) ends inls.sumOf { it.fraction ?: 0.0 } / ls.size, andls.sizeis now N+1. The bar is systematically understated — a single layer at 100% renders as 50%.quiet = layers.size - shown.sizeinUpdateProgressCardcounts the rollup, so "N layers already present or queued" is off by one.Before this PR
kind: "pull"failed to decode and the event was dropped, so the miscount is introduced here rather than uncovered. DropPULLfromlayers; if the rollup is worth rendering it needs its own bucket that reads thepullobject.2. A null snapshot leaves the button spinning forever
SchemaViewModel.kt:833-846core:task:*'s snapshot returnsnullwhen the run is not found (routes.ts:230-233:return run ? projectRun(run, viewer) : null).decodeFromString<TaskRun>("null")throws,getOrNull()swallows it,return@collect— andrunningActionis never cleared. The docker reference handles exactly this case:Same class of hole: a rejected subscription.
LiveClient.kt:234removes the sub fromsubsand logs a warning — the flow then goes silent forever and is not re-subscribed on reconnect. Both paths need to reachfinishRun(or an equivalent failure state) instead of going quiet.3.
followRun's job is never cancelledSchemaViewModel.kt:870puts it inrefreshJobs, which is only cleared bystart()on a page change. So the subscription survives the run:finishRunfires a second time, andrefetchAll()runs again.Cancel the job from
finishRun, or hold it in a dedicated field the wayprogressJobis held.Should fix
4.
phaseis carried forward but never readUpdateProgress.kt:185-190Download bytes are compressed and extract bytes are not, which is why they must not be summed — and the web feed filters on exactly that (
update_feed.svelte.ts:113):Mobile still adds both. Carrying
phasethrough the merge is a precondition for the fix, not the fix; the PR description claims the latter.5.
body?.jsonObjectmoved onto the success pathSchemaViewModel.kt:777modulePostreturns anyJsonElement(ApiClient.kt:113parses whatever came back)..jsonObjectthrowsIllegalArgumentExceptionon a non-object body, and this line sits outside therunCatching— uncaught inviewModelScope, so it takes the app down. Previously it only ran in thecode >= 400branch, where the body is always{error}. No core module returns a non-object from a POST today, so this is latent, but(body as? JsonObject)costs nothing.6. Two actions in flight cross-talk
_state.progressandrunningActionare single-valued. A second action started while the first is running shares the feed, and the first run's terminal status clears the second's spinner. The shape predates this PR, but withprogressTopicthe window was seconds; adist-upgrademakes it thirty minutes.7. A dropped frame is now unrecoverable
LiveClient.kt:225-231advancessub.tseqbeforeemit, andtopic()is acallbackFlow— 64-slot buffer,trySend. An event dropped by buffer overflow still consumes itstseq, so the reconnectsincewill not ask for it back, and this PR removes theResyncthat was the previous fallback.module_updatesnow emits one frame per apt line, which is the rate that overflows. Advancetseqonly on a successful send.Notes
finishRuntreatsskippedas success. That is right foronSelfConflict: drop, butskippedalso arrives from a gate that can never open (whenMissing: "skip"), where it means the work silently did not happen.feed.lineshas no cap, andUpdateProgressCardrenders oneTextper line inside a single non-lazyColumn.ProgressFeed.ofalone seeds it with up toMAX_LINES = 500from the snapshot, before any deltas.RefreshPolicy.gate's doc does say not to gate user-initiated flows, so leavingfollowRunungated is correct and matches theprogressTopicpath.All seven addressed in
a2b6dec, plus the two notes. Where the fix differs from the one suggested, why:Blockers
1.
PULLis not a layer — dropped fromlayers, and it is in no bucket at all now (resourcesalready excluded it), which is whereupdate_feed.svelte.tsputs it. The aggregate mean and the "already present or queued" count are both over real layers again.2. Null snapshot / rejected subscription —
decodeFromString<TaskRun?>distinguishes the three outcomes: a run,null(pruned or never existed), and a body that would not parse. The last two end the action with a message rather than going quiet. For the rejected subscription the fix had to go intoLiveClient: a new terminalLiveEvent.Rejectedis emitted before the sub is dropped, since a caller waiting on the topic for something to finish otherwise waits forever.startLogFollowsignores it deliberately — that tail is a live extra on a source that is fetched and refreshed anyway.3.
followRun's job — own field, cancelled by a singleendRunthat every terminal path goes through, so no secondfinishRunwhen a reconnect re-snapshots a run that already ended.Should fix
4.
phasecarried but never read — filteringsized()to the download phase is not sufficient on its own here: a layer stops reporting its download figure the moment it starts extracting, so the sum shrinks through the pull and reads 0 at the end (update_feed.svelte.tsdoes not hit that becausedownloadedprefers the rollup and only falls back to the layer sum). SoPullSummaryis now parsed off thekind: "pull"entity andtransferred/transferTotalread it, with the download-phase layer sum as the fallback for a server too old to send the rollup orphase— where every sized layer is a download layer and the two readings agree. That is also the answer to "if the rollup is worth rendering it needs its own bucket".5.
body?.jsonObject—as? JsonObject/as? JsonPrimitive.6. Two actions in flight —
runActionandopenActionreturn early while one is running. The buttons were already disabled; this makes single-flight a property of the model rather than of the screen, which is the part that matters once a run lasts half an hour.7. Dropped frame —
Sub.emitreports whethertrySendlanded.tseqadvances only for a delivered event and then freezes at the first drop (Sub.gap), because it is a promise that everything through it arrived — advancing past a hole to the next delivered event would still ask the server to skip what was lost. The next snapshot clears it. A dropped snapshot no longer adopts its owntseqeither: resuming from it would resume onto no baseline at all.Notes
skipped— still finishes the button, but the run'sreasonis pushed into the feed, so a gate that will never open says so instead of looking like a clean success.feed.lines— capped at 500, the window core keeps (tasks/types.tsMAX_LINES), inProgressFeedrather than at the render site so the snapshot path is capped too.RefreshPolicy.gate— unchanged, as agreed.One thing the review did not catch
app/src/test/java/.../ProgressFeedTest.ktdid not compile on this branch: the newpush(ProgressEntity)overload madejson.decodeFromString(...)ambiguous at four call sites. OnlyassembleDebughad been run, and CI runstestReleaseUnitTest— so this would have gone red on merge. Call sites made explicit;./gradlew testDebugUnitTestis green (14 inProgressFeedTest, including new ones for the rollup, the two phases and the line cap),assembleDebuggreen.Still not exercised against a live server —
module_updatesremains the companion PR.Reviewed at
a2b6dec, cross-checked against coref3426be(packages/sdk/tasks.ts,server/src/tasks/{types,projection,routes}.ts,events/live.ts, dockercompose_progress.ts). Not built locally, so theassembleDebugclaim is taken on trust.The design holds up: response-decides-protocol, retained topic, terminal status ends the action.
MAX_FEED_LINESmatches core'sMAX_LINES,ProgressFeed.pushmatches the SDK'smergeProgressfield for field,RunStatusmatches the SDK 1:1,coerceInputValuesreally is on inApiClient,core:task:*carries noroleso a non-admin does follow their own run, and skippingRefreshPolicy.gatematches that function's own doc. Thenullsnapshot path is right.What I want changed before this merges is not the design — it is what happens when the channel misbehaves.
1. The action lifecycle now depends entirely on the WebSocket, with no fallback and no escape. On
{runId}the button is released only by a terminal status offcore:task:<id>. If the live channel never connects — a proxy that stripsUpgrade, a blocked WS, an older server —runningActionstays set for the life of the page: every action button disabled, no timeout, no cancel, no REST fallback. Before this PR a 2xx released the button, so this is a new way to wedge the screen. Core already servesGET /api/core/tasks/:id, andLiveClient.connectedis aStateFlow.2.
submitFormstill has the exact bug this PR exists to kill. A form whose submit route answers{runId}reports success and refetches the instant the run is created. The PR states that the response decides the protocol and no schema field selects it — which makes the form path a live trap, not a hypothetical one.3.
Sub.gapnever re-baselines. It is cleared only by asnap, whilesubFramekeeps sendingsince. After one dropped frametseqfreezes for the rest of the connection, so every later reconnect replays events already shown — keyed entities are idempotent, log lines are not, and the duplicate window grows with the run. A dropped terminalstatusframe also means the button spins until the socket happens to drop.Details in the inline comments, along with the smaller ones (unknown status, page-wide button lock, invisible
waiting, unboundedresourcesrows, no tests for the new state machine).@ -0,0 +42,4 @@val reason: String? = null,val error: String? = null,val result: JsonElement? = null,val finishedAtMs: Long? = null,finishedAtMsis decoded and never read anywhere, and three lines of class doc (andisTerminalbelow) explain thatUNKNOWNdeliberately reads as still-running — meaning a status core adds later leaves the app spinning forever, since no further events arrive on a topic whose run has ended. "At least the run is on screen" undersells the cost: the whole page's action buttons stay disabled with no way out.The field right here closes that hole with data already on the wire:
A run core has stamped
finishedAtMson is over, whatever the app makes of its status string.@ -127,0 +217,4 @@*/val resources: List<ProgressEntity>get() = order.mapNotNull { items[it] }.filter {it.kind != ProgressKind.LAYER &&This is now an open-ended catch-all — RESOURCE, STEP, OTHER, and any string a module invents — and core keeps up to
MAX_ENTITIES = 400entities per run.UpdateProgressCardrendersresourceswith no cap, in a plainColumninside a singleLazyColumnitem, on top of up to 500Textlines from the tail. Layers getMAX_LAYER_ROWSand a "+N more" line precisely to avoid that; these need the same treatment.The docker case was self-limiting (containers, networks, volumes). Opening the bucket to every unknown kind removes that accident.
@ -129,0 +231,4 @@* counted, which is what that server meant.*/private val downloadLayersget() = layers.filter { it.total != null && (it.phase == null || it.phase == "download") }Worth being precise in the doc: splitting by phase stops the figure exceeding its own total, but it does not stop it walking backwards — a layer that moves to
extractdrops out of bothtransferredandtransferTotal, so "X of ~Y" shrinks. The new testwithout a rollup only the download phase is summedasserts exactly that (1.0e8 of 5.0e8 while 2.0e8 has already been extracted).No producer hits this today — docker emits the rollup and
phasetogether — but the comment reads as though the fallback is sound, and the next module to emitphasewithout a rollup will inherit the bar that goes down.@ -178,0 +260,4 @@if (!sub.gap) {Log.w(TAG, "event on \"${sub.topic}\" dropped: collector fell behind")}sub.gap = trueBlocking —
gapnever clears, so one dropped frame duplicates every line after it.gapis cleared only by asnap, andsubFramekeeps sendingsinceregardless. So after a single drop:tseqfreezes at the last delivered event for the rest of the connection;Keyed entities survive that (a repeat replaces the row), but
pushLineappends, so the run's text tail grows a duplicated block whose size scales with how long the run continued after the drop.Two changes fix it, and the second matters more:
gapis set, omitsince/epochinsubFrameso the server answers with a fresh snapshot —ProgressFeed.ofis built to consume exactly that, and it re-baselines the hole instead of papering over it;since) right here recovers within the same connection. As written, a dropped terminalstatusframe leaves the button spinning until the socket happens to break — which on a healthy connection may be never.@ -248,1 +345,4 @@put("topic", sub.topic)// resuming a retained topic: the server replays the gap, or sends a// fresh snapshot when it no longer holds it (or the epoch moved)sub.tseq?.let {This is the other half of the
gapproblem:sinceis sent whenevertseq != null, including whengapis true — i.e. precisely when the client's "I have everything through here" promise is known to be false for everything after that point. Gate it:A missing
sincemakes the server re-snapshot, which is the correct answer for a subscription that has a hole in it.Minor, same function: when
epochis null buttseqis not (the malformed-frame path that setstseq = 0L), this sends"epoch": null. Core comparesepoch === state.epoch, so that is a guaranteed miss and a guaranteed snapshot — correct, but worth a word in the comment, since the0Ldefault is doing something subtler than it looks.@ -731,0 +760,4 @@// The buttons are disabled while one runs; this makes that an// invariant of the model rather than a property of the screen — with// a run that lasts half an hour the window is no longer a blink.if (_state.value.runningAction != null) returnThe invariant is right, but the screen-side consequence has outgrown its original sizing.
ActionsRowrendersenabled = running == null(SchemaScreen.kt:559), so for the whole run — half an hour, by this comment's own example — the page disables every action, including navigation-only entries andopendownloads, neither of which touches the run or takes a server-side lock. What used to be a blink is now the run's lifetime.Two things worth doing:
pathactions. Navigation andopenhave no shared state with the run.capabilities.cancelon the projected run andPOST /api/core/tasks/:id/cancel; this PR decodes neither. Even without cancel, a "stop following" that callsendRun(null)beats a page that can only be escaped by navigating away.@ -775,3 +813,4 @@actionError = msg ?: "action failed (HTTP $code)",)progressJob?.cancel()} else if (runId != null) {Blocking — the same protocol switch is missing on the form path.
The comment here is right that a response either carries a
runIdor it does not, and that no schema field selects it. That reasoning applies verbatim tosubmitForm(~line 1017), which ignoresrunIdentirely: a form whose submit route starts a host-owned run reports success and callsrefetchAll()the instant the run is created — the silent success this PR describes as the worst possible failure mode, left standing on the other write path.Either route it through the same
followRun, or say in the PR body why forms are deliberately out of scope for this pass.While you are in there:
submitForm's 4xx branch still readsbody?.jsonObject?.get("error")?.jsonPrimitive?.content, the unguarded cast you just replaced here withas?, and it also runs insideresult.foldoutside therunCatching— so a valid non-object JSON error body throws inviewModelScope, which is the crash your comment above describes.@ -794,0 +856,4 @@* empty with their names in `withheld`. That is deliberate and needs no* handling here — the status still lands, so the button still finishes.*/private fun followRun(runId: Long) {Blocking — no fallback and no escape when the channel is down.
After this, the only thing that clears
runningActionis a terminal status offcore:task:<id>. If the live channel never connects — proxy stripsUpgrade, WS blocked on the network, server older than the retained topic — the subscription never lands, nothing is ever emitted, and the page stays wedged: spinner forever, every action button disabled, no timeout, no cancel, no REST read. Before this PR a 2xx released the button, so this is a new failure mode rather than an inherited one, and it is invisible to the user (the run may well be running fine).Core already serves
GET /api/core/tasks/:idandLiveClient.connectedis aStateFlow. Either is enough:connected.value == false, orendRun("lost contact with run #$runId")if nosnaphas arrived after a few seconds.The watchdog is the cheaper of the two and covers the "server does not speak this yet" case as well.
@ -794,0 +937,4 @@}endRun(if (run.status.isFailure) {run.error ?: run.reason ?: "the run ${run.status.name.lowercase()}"Nit: this renders "the run cancelled" and "the run interrupted".
failedis the only status that reads as a verb — the other two need "was".@ -794,0 +908,4 @@progress = _state.value.progress.pushLine(it),)}"status" -> ev.run?.let { if (it.status.isTerminal) finishRun(it) }Only terminal statuses do anything here, so a run parked on a gate or behind a lock (
waiting) is pixel-identical to one doing work, potentially for a long time — and gates are exactly the feature that makes that wait long. The snapshot already carriesstatusandtitle; surfacing the non-terminal transitions (or just seeding one line from the snapshot whenstatus == WAITING) is the difference between "nothing is happening" and "nothing is happening yet, because X".Same branch:
"step"events are dropped, andRunStepis decoded but never rendered. For an apt run the steps are the only structure the operator gets between log lines. Fine to defer, but it is worth saying in the PR body which of the five event types are deliberately ignored.@ -156,2 +168,4 @@}@Testfun `the pull rollup is not a layer`() {These are good tests, but they cover the part that was already covered — pure
ProgressFeedarithmetic. Everything this PR actually risks is untested:LiveClient'sSubstate machine:tseqadoption onsnap,tseqadvance only on delivered events,gapset on a dropped event,Resyncsuppressed for retained subs,since/epochon the resubscribe frame. It is plain JVM code reachable by feedinghandle()frames — no Android, no socket.ProgressFeed.of, which is the entire retained path's baseline and has no test at all (orderdistinct vsassociateBylast-wins,linestail).followRun's terminal paths:nullsnapshot,Rejected, terminal-on-snapshot vs terminal-on-status, and that neither ends the run twice.The first of those is where the two remaining correctness bugs live, which is not a coincidence.
Addressed in
641b860. Point by point, in your numbering.1. No fallback, no escape when the channel is down. Both, in the end — the watchdog alone would have turned "the WS is blocked on this network" into "every run on this network fails", which is a different lie.
followRunarms a 10s watchdog against the first frame off the topic (a retained subscription answers with its snapshot immediately, so silence that long is not a quiet run). It fires intopollRun, which readsGET /api/core/tasks/:idevery 3s and drives the sameshowRun/finishRun/endRunpaths — a 404 ends the follow with "no longer on the server", a transport error ends it with what the error said. Any frame from the channel cancels the watchdog, so the two never race.The escape is separate and always present:
capabilities.cancelis decoded (asJsonPrimitive, since the wire union is"cooperative" | falseandString?would fail to decode a plainfalse), "Cancel run" posts to/canceland does not release the button — cancellation is cooperative, so the run's own terminal status still ends it — and "Stop following" callsendRun(null).Same comment's other half:
ActionsRownow disables per action.pathactions block onrunning,openactions block on a new single-valueddownloading, navigation blocks on nothing.openActionno longer borrowsrunningActionat all, so a half-hour run and a download coexist.2.
submitForm. It readsrunIdoff the response and follows the run, on exactly the same rule asrunAction— the response decides, no schema field. It deliberately does not open thesuccessPagewhen it does: navigating away cancels the follow (startclearsrunJob), which would restore the silent success this branch exists to remove. Noted in the PR body. The 4xx branch'sbody?.jsonObject?...is now(body as? JsonObject)/(… as? JsonPrimitive).3.
gapnever re-baselines. Both of your changes, plus a third that fell out of them.since/epochare withheld whilegapis set, so the server re-snapshots. The re-subscribe (unsub + sub) goes out synchronously at the drop, on the same connection, rather than waiting for a reconnect. And since the burst that overflowed the buffer can perfectly well swallow the new snapshot too, a droppedsnapclears the "re-subscribe in flight" flag and the first delivered event after it — proof the collector caught up — sends another. One re-subscribe per hole either way, never one per event.The state machine moved to
SubState, which is plain JVM code:LiveClient.Subholds one andsubFrame(id, topic, state)is a free function.SubStateTestcovers tseq adoption onsnap, thetseq = 0malformed path, advance only on delivered events, gap set, the re-subscribe and its retry,plain(which is what suppressesResyncfor retained topics), and the frame'ssince/epoch.Smaller ones.
finishedAtMs:TaskRun.isOverisstatus.isTerminal || finishedAtMs != null, used at both call sites.RunStatus's doc no longer claims a spinner is the price ofUNKNOWN.waiting:runStatusis carried in the UI state and the card renders "Waiting to start — held behind a gate or another run" and "Cancelling — waiting for the run to stop". The card draws on a non-null status even when the feed is empty, which is exactly the gated case.step: still dropped, and now listed in the PR body along withcheckpointunder what is deliberately unhandled.RunStepstays decoded because the snapshot carries it. Worth a follow-up for apt runs, as you say.resources: capped at 8 with a "+N earlier" line, taking the last 8 — at minute twenty the first packages are not the interesting ones.downloadLayersdoc: says outright that splitting by phase stops the figure exceeding its total but not walking backwards, and thatpullbeing the preferred source is why no producer hits it today.TaskRun.endMessageas awhenper status rather than an interpolated status name, and is tested.Tests.
SubStateas above;ProgressFeed.of(order distinct, last-wins by key, the 500-line tail); and what ends a run —nullsnapshot decoding tonullrather than throwing, unknown status reading as live,finishedAtMsoverriding it, and the failure wording.followRun's coroutine wiring is still untested: the ViewModel wants anApplicationand the container, and the module has neither Robolectric norcoroutines-test. Adding one of those is a bigger call than this PR, so I have said so in the body rather than pretending the gap is closed../gradlew assembleDebugandtestDebugUnitTestgreen (67 tests). Still not exercised against a live server.Reviewed at
641b860. Line refs are against that commit../gradlew testDebugUnitTestis green.What lands well
{runId}vsprogressTopic), not a schema field. Right call — no schema migration, old servers keep working, deprecation documented in README.isOver = status.isTerminal || finishedAtMs != null. Correct instinct for a status this build cannot name, and tested.LiveEvent.Rejected— a refused subscription used to go silent forever. Real bug closed.downloadingsplit out ofrunningAction. A 30-minute run no longer freezes every download button on the page.SubStateextracted so the reconnect story is testable without a socket.SubStateTestis the strongest part of the PR.Blocking
H1 —
submitFormhas no "one run at a time" guard; form submit hijacks a live runrunActiongot the guard (SchemaViewModel.kt:789) andActionsRowdisables posting actions onrunning != null.submitForm(:1092) got neither —FormCardreceives onlysubmitting = state.submittingForm(SchemaScreen.kt:272).So while a host-owned run is followed, every form on the page stays submittable.
followRun(:1182) then doesrunJob?.cancel()and overwritesrunId/progress/runningAction. Run #1 keeps executing host-side, unobserved, outcome never reported — the exact silent lie the KDoc at:1167says this path exists to stop.Fix: same guard in
submitForm, and feedstate.runningActionintoFormCard's disabled condition.H2 — dropped-snapshot path still wedges the screen at end of run
SubState.gapclears only inonSnapshot(SubState.kt:758). Sequence:callbackFlowbuffer →onEvent(delivered=false)→ re-subscribe.onSnapshotDropped()clearsresubscribed.onEvent's return — i.e. on the next delivered event.If the run went terminal inside that burst, there is no next event. Collector never sees the terminal status;
followRun's watchdog was already cancelled by the first frame. Spinner forever — the failure this branch's last commit ("every way a followed run could wedge the screen") claims to have closed. An apt run pushing lines faster than the Main-dispatcher collector copies_statereaches 64 easily.Fix, pick one: re-arm the silence watchdog after every frame instead of cancelling it permanently; give the task topic an unbounded buffer; or drive the re-subscribe off a timer, not off the next event.
H3 — watchdog is one-shot, covers subscribe-time silence only
watchdog.cancel()at:1119-1121fires on the first frame and never comes back. After that there is no REST fallback: socket dies, reconnect backs off toMAX_BACKOFF_MS, or the server just stops publishing — nothing ends the follow.LiveClient.connectedis exposed and unused. Same fix as H2.Medium
M1 — one unknown entity kills the whole follow.
ProgressEntityrequireskey/id/status(UpdateProgress.kt:21-27),RunSteprequiresname/status(Task.kt:205). One entity missing any of them failsdecodeFromString<TaskRun?>→endRun("could not read the state of run #N")(:1144-1148). Contradicts the PR's own forward-compat stance three files over (UNKNOWNstatus,OTHERkind).stepsis decoded and never rendered — it can only lose. Default those fields, or dropstepsfrom the model.M2 —
pollRunquits on the first transient failure.:1199-1203. One Wi-Fi→LTE handover ends the follow of a healthy run. Needs a retry budget before declaring loss.M3 —
pollRunpolls 3s forever, background included. No backoff, no cap,viewModelScopeis not lifecycle-aware. 2h run ≈ 2400 requests plus radio wakeups with the screen off. Back off 3s→15s→30s and pause when not resumed.M4 —
snap/evdon't checkws === socket.LiveClient.kt:199,219. Every other handler does (hello:181,ping:197,scheduleAck:281,armDeadTimer:270). A late frame from a dead socket can callonSnapshot/onEventand overwritetseq/epochfor a subscription already resumed on the new socket — the nextsincethen asks the server to skip events that were never delivered. Silent hole, precisely the class this file exists to prevent.M5 — reconnect snapshot drops the merge carry-forward.
ProgressFeed.of(UpdateProgress.kt:341) rebuilds items verbatim;pushcarriestotal/current/fraction/phaseforward for the "Pull complete carries no bytes" case (:183-190). After a reconnect — or once the REST poll takes over — that is gone, so "X of ~Y" and the bar jump backwards. Either the server guarantees compacted entities keep last-known sizes, orof()applies the same merge.M6 —
pushcarriesphaseforward but notpull.:367carriesphasespecifically so counts aren't misattributed.pullgets no such treatment, so a rollup frame that omits it wipes the summary and flipstransferred/transferTotalfrom rollup to layer-sum mid-pull — the same backwards jump the rollup exists to prevent.M7 — the rollup elvis fires on null, not on zero.
pull?.downloadTotal ?: downloadLayers.sumOf{…}(:258).PullSummary.downloadTotaldefaults to0.0, so a rollup arriving before totals are known suppresses the whole pull section (if (total > 0)) while layers are already reporting bytes.takeIf { it > 0 }is the honest read.M8 — the log tail renders all 500 lines in a non-lazy Column.
feed.lines.forEach { Text(…) }(UpdateProgressCard.kt:155) inside oneLazyColumnitem.MAX_RESOURCE_ROWS = 8was added for exactly this reason — "renders in a plainColumninside oneLazyColumnitem, which composes every row it is given" — and then the line ceiling went from "a handful" to 500 with no render cap.feed.servicesis uncapped too, against core's 400-entity budget. Also no autoscroll: the newest line pushes itself off-screen.M9 —
withheldis decoded, argued for at length, and never shown.Task.kt:22insists "an empty list and a withheld one are different facts — 'no output' is not 'not for you'", README repeats it, then the card renders a withheld run identically to a silent one. Surface it or delete the field and the paragraph.Low
canCancelrequires the literal"cooperative"(Task.kt:154). Every other unknown value in this PR degrades forward; this one silently drops the cancel button if core names a second mechanism.cancel is JsonPrimitive && content != "false"matches the file's own stance.cancelRunclearsrunCancellablepermanently on any refusal (:1240), transient 5xx included — no retry for the rest of the run.actionError, notformError— banner is nowhere near the form. AndsuccessPageis never opened even onSUCCEEDED; the KDoc justifies not navigating during the run, not after.stopFollowing()→endRun(null)wipes any unrelatedactionErroralready on screen.runIdis not inSavedStateHandle. Navigating away or a process kill orphans the run. "Backgrounding the app and returning resumes rather than loses it" (:878) holds only while the ViewModel lives — narrow the claim or add the restore.progressJobstays inrefreshJobs(:825,:848) — one dead Job per press for the page's life.followRunand the first frame,runId != nullwith empty feed and null status: "Cancel run" / "Stop following" render with nothing above them.pullSettledrequireslayers.isNotEmpty()— an all-cached pull (rollup, zero LAYER entities) reads "Pulling images" forever._state.value = _state.value.copy(…)throughout, while:1131documents exactly why_state.update {}is needed. Main-thread-only today, but the hazard is already written down in the same file.SubStatehas no thread-safety note. Every access is underLiveClient.lock; nothing in the class says so, and it'sinternal.Tests
SubStateTestcovers the right cases — "one re-subscribe per hole" and "a re-subscribe whose snapshot also dropped is sent again" are the two that matter.TaskRunTestcovers the unknown-status escape hatch properly.Gap:
followRun's state machine — watchdog,Rejected, double-follow,endRunidempotence — has zero coverage, and H1/H2/H3 all live there. No ViewModel test exists anywhere in the repo, so this is a new fixture rather than a missing one.SubStateTeststructurally cannot reach H2: the retry trigger sits inLiveClient's frame loop, not inSubState.Note
Version bump 0.30.0 → 0.31.0 is correct per CLAUDE.md (new feature, pre-1.0 minor). Commit messages are Conventional.
A large share of these +1326 lines is KDoc prose, several blocks running 15+ lines. The content is genuinely why rather than what, which is the good kind — but
Task.ktandfollowRunnow carry design-doc paragraphs that will drift out of date faster than the code they sit on. M9 is already an instance: the doc asserts a behavior the code does not have.Third review, all findings: - H1: submitForm refuses while a run is followed and FormCard's button disables — any submit's response may claim the single-valued run state, so a live follow must not be hijackable from a form. - H2/H3: the silence watchdog is re-armed on every frame instead of dying at the first one. After FOLLOW_SILENCE_MS of channel silence the run is reconciled over its REST twin, backing off to 30s, tolerating 3 consecutive failures (M2) and skipping the radio while the app is backgrounded (M3). A dropped terminal frame or a socket that dies mid-run now ends the follow within one probe. - M1: ProgressEntity and RunStep default every field — one degraded entity inside a snapshot no longer ends the follow. - M4: snap/ev check ws === socket like every other handler. - M5/M6: ProgressFeed.of merges the snapshot against what the screen already knew — total/current/fraction/phase/pull carry forward, so a reconnect's re-snapshot cannot walk the bars backwards. - M7: the rollup is believed only once downloadTotal > 0. - M8: the card renders a 30-line log tail and caps service rows. - M9: withheld field names are shown instead of reading as a silent run. - L1: any cancel mechanism name offers the button, not just the one this build knows. L2: a refused cancel keeps the button. L3: a form-started run reports into formError and opens successPage on success. L4: "stop following" no longer wipes unrelated banners. L5: the resume claim is narrowed to the ViewModel's life. L6: progressJob left refreshJobs for its own field. L7: the card draws before the first frame so the run buttons sit on something. L8: an all-cached pull settles on the rollup's own state. L9: run paths write state via update{}. L10: SubState documents the lock it lives under. 75 unit tests green (8 new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>All findings from the third review addressed in
4950da3.Blocking
submitFormnow has the same one-run-at-a-time guard asrunAction(model invariant), andFormCardgetsrunInFlightso every submit button on the page disables while a run is followed.FOLLOW_SILENCE_MSof channel silence it reconciles the run overGET /api/core/tasks/:id; a delivering channel resets it, a dead one turns it into the follow. So the dropped-snapshot burst (H2) and the mid-run socket death (H3) both end at the next probe — the retry no longer waits for a delivered event that may never come.Medium
ProgressEntityandRunStepdefault every field; one degraded entity in a snapshot degrades that entity, not the follow.stepsstays decoded (it still arrives inside the snapshot either way).ProcessLifecycleOwnersays the app is not STARTED (the timer keeps running, the radio stays asleep).snap/evnow checkws === socketbefore emitting or touchingSubState, same as every other handler.ProgressFeed.of(run, prev)merges each snapshot entity against what the screen already knew, through the same mergepushuses; entities absent from the snapshot are still dropped (the snapshot stays the whole truth).pullforward alongsidephase.sizedPullgates the rollup ondownloadTotal > 0; null and zero both mean "nothing to read yet, use the layer sum" — numerator and denominator switch sources together.+ N earlier lines) and caps service rows at the same budget as resources. The tail cap is also the autoscroll fix: the newest line can no longer push itself off-screen.withheld, which the snapshot now carries into the feed. README updated to match.Low
false/absent deny.formError, andsuccessPageopens afterSUCCEEDED(deferred, not dropped — the KDoc now says so).endRun(null)preserves existing banners.SavedStateHandle; happy to add the restore as a follow-up if you want it.)progressJobis its own field, cancelled on page change likerunJob.runId != null, with a "Following the run — waiting for its first report" line under it, so the buttons sit on something.pullSettledfollows the rollup entity's own state._state.update {}.SubStatedocuments the lock contract.Tests
8 new: any-mechanism cancel, degraded entity/step decode,
withheldinto the feed, re-snapshot merge carry-forward (and that absent entities still drop),pullcarried across a rollup frame that omits it, zero-total rollup deferring to the layer sum, all-cached pull settling. 75 total, green;assembleDebuggreen.The
followRuncoroutine machine itself (watchdog re-arm, probe backoff, double-follow) is still not unit-tested — that needs the ViewModel fixture (Robolectric or a container seam) this module doesn't have yet. Say the word and I'll set one up as its own PR rather than growing this one further.Re-review at
4950da3../gradlew testDebugUnitTestgreen.All three blocking items are genuinely closed — I read the code, not just the commit message. Dropping the REQUEST_CHANGES. What's below is new-and-small plus one carry-over; nothing I'd hold the merge on.
Verified closed
submitForm:1218has the guard,FormCard.runInFlight(FormCard.kt:308, wired atSchemaScreen.kt:271) disables the button. Model invariant and screen, same as the actions.:1018) instead of cancelled once, so a socket that dies mid-run, and a burst that swallows the terminal status, both reconcile over REST withinFOLLOW_SILENCE_MS. Backoff to 30s and theProcessLifecycleOwnerforeground check (:1001) answer M3 properly. Worth noting explicitly:SubState's own hole still can't self-heal without a further event — the probe is what saves it, not the live state machine. Fine as a design, just don't lose track of which layer holds the guarantee.RunStep/ProgressEntityfully defaulted, with the "wrong row beats a dead screen" trade written down. M2 three-failure budget. M4ws === socketon bothsnapandev, including theonSnapshotDroppedpath. M5/M6mergeextracted and applied toof(),pullcarried forward withphase. M7sizedPull. M8MAX_LOG_ROWS, services capped. M9 the card names withheld fields. L1–L4, L6–L10 all addressed; L5 narrowed the KDoc claim rather than persisting the id, which is a reasonable call to make explicitly.The
merge-against-previnProgressFeed.ofwith "entities absent from the snapshot are dropped, as they must be" is the right resolution of M5 — it takes the snapshot as authoritative for membership and the screen as authoritative for what only it still knows. The test atTaskRunTestpins both halves.New
N1 —
runCatchinginprobe()swallowsCancellationException.:963. Every frame callsarmWatchdog()→silence?.cancel()(:996), which cancels an in-flight probe.taskRun'swithContextthrowsCancellationException,runCatchingcatches it, and:967counts it as a network failure.It can't currently reach the threshold — the collector sets
probeFailures = 0at:1016before the cancel lands, so at most one bogus increment per frame. But the budget is left dirty (a genuinely dead link then needs 2 real failures, not 3), and the pattern is a trap for whoever touches this next..onFailure { if (it is CancellationException) throw it }, orcurrentCoroutineContext().ensureActive()after therunCatching.N2 — a slow probe response can overwrite newer live state.
probe()→showRun(run)(:989) with no sequencing check. Probe fires after 10s of silence; a frame landing during the REST round-trip updates the feed, then the older snapshot replaces it —lineswholesale, andrunStatuscan regress (live "cancelling" → stale "running"). Self-corrects on the next frame, and terminal is safe (endRuncancels the probe's parent), so it's a flicker rather than a wedge.finishedAtMs/a monotonic stamp, or just skippingshowRunwhen a frame arrived after the probe started, closes it.N3 — per-frame Job churn on the hot path.
armWatchdog()cancels a coroutine and launches a fresh one for every frame (:1018). On an apt run that's a job allocation per log line, on Main. The_statecopy per line already dominates, so this isn't the bottleneck — but alastFrameAtMstimestamp checked by one long-lived loop gets the same behavior with no allocation.N4 —
runOnSuccessholds a composable's lambda in the ViewModel for the life of the run.:271, assigned at:1310from theonSuccessclosureSchemaScreenrecreates on every recomposition. It capturesonNavigateSub, and it's invoked minutes later at:1157. Storing the interpolatedsuccessPageinSchemaUiStateas a one-shot event and letting the screen navigate keeps the nav lambda out of the ViewModel.N5 — services take the head, resources take the tail.
UpdateProgressCard.kt:141vs:171. The resources comment argues the tail is right because the newest rows are the ones still moving; services gettake()with "same budget as resources". Either the argument applies to both or it applies to neither — and the services overflow reads "+ N more" where the resources one reads "+ N earlier", so the two disagree about which end got cut.N6 — a defaulted
keyquietly eats the legacy text fallback. WithProgressEntity.keynow defaulted, a legacyprogressTopicframe carrying a malformedprogressobject decodes instead of throwing, sorunAction's "fall back to the rawmsgas a plain text line" path (:851) no longer runs — the entity becomes an empty-key row and the message text is dropped. Narrow, and the M1 trade is still worth it; just no longer true that a bad payload always shows up as text.Still open
The ViewModel test gap from the last round.
followRungrewprobe(),armWatchdog(), the failure budget, the foreground check and the form-run success routing — that's more untested state machine than it had when I first flagged it, and N1/N2 both live there.ProgressFeedTest/TaskRunTestcontinue to cover the pure parts well (the re-snapshot merge test and the all-cached rollup test are the right shapes), but nothing exercises the coroutine.No
versionNamebump in this commit. Reasonable while 0.31.0 is unreleased on the branch — flagging only so it's a decision rather than an oversight.Full review of the PR at
4950da3— whole diff against473ef61, not just the last commit. Supersedes the two earlier passes; this one stands alone../gradlew testDebugUnitTestgreen.Everything from the first round is genuinely fixed (I read the code, not the commit messages). What follows is what a complete read of the final state turns up, including two things the first pass missed and one regression the H1 fix introduced.
The design
Right calls, worth stating because they're the parts that should not be traded away later:
{runId}vsprogressTopic, no schema field. No migration, old servers keep working, and both call sites (runAction,submitForm) read it the same way.isOver = status.isTerminal || finishedAtMs != null. A status this build cannot name reads as running; a finish time overrides it. That's the correct asymmetry and it's tested.ProgressFeed.of(run, prev)gets the hard part right: the snapshot is authoritative for membership, the screen stays authoritative for what only it still knows (a finished layer's byte counts). "Entities absent from the snapshot are dropped, as they must be" is the line that makes it coherent.SubStateextracted. The reconnect story is the whole correctness story and it's the one piece testable without a socket.SubStateTestis the strongest file in the PR.Blocking
B1 — the run lock is asymmetric: actions are unguarded while a form submit is in flight
The H1 fix locked one direction only.
submitForm:1218guards onrunningAction != null, andFormCard.runInFlight(SchemaScreen.kt:271) disables the forms while an action runs. ✅runAction:826guards onrunningAction != null— but a form submit setssubmittingForm, notrunningAction.runningActionis only set once the response comes back carrying arunId(:1305).So for the entire duration of the submit request — a multipart upload, so potentially minutes — every posting action on the page is enabled and
runAction's guard waves it through. Both responses can carry arunId; the secondfollowRundoesrunJob?.cancel()and overwritesrunId/progress/runningAction. First run keeps executing host-side, unobserved, outcome never reported. That is H1 verbatim, with the arrow reversed.The confirm dialog inherits it:
SchemaScreen.kt:519callsrunActionwith no re-check, so a dialog opened before the submit confirms straight into the hole.Fix: both guards need to test both fields. A single derived
runLocked = runningAction != null || submittingForm != nullused byrunAction,submitForm,ActionsRowandFormCardmakes the invariant one thing instead of four.B2 — a wedged legacy
progressTopicrun now freezes the forms toorunInFlight = state.runningAction != nullis new, and the legacy path setsrunningActionas well.A
progressTopicaction ends only when a line starts "done" or "error" (:851-857). If the module's wording drifts, or the last frame is lost — at-most-once, no replay, which is the whole reason this PR exists —runningActionnever clears. Before this PR that disabled the action buttons. Now it disables every form on the page too, for the life of the page, with no way out:state.runIdis null for a legacy run, so the "Stop following" escape hatch (SchemaScreen.kt:244) never renders.The PR's own thesis is "a followed run must always have a way out". The legacy path is the one that most needs it — it's the transport that can't recover — and it's the only one that doesn't get it. Either show the escape hatch whenever
runningAction != null, or give the legacy path the same silence watchdog that now protects the host-owned one.B3 —
TaskRun.idis the last required field, and a status delta that omits it is dropped silentlyThe last commit correctly defaulted every field of
RunStepandProgressEntity, with the reasoning written down: one required field a future core omits fails the whole decode.TaskRun.id(Task.kt:28) is still required, andTaskEvent.runis aTaskRun.So a
{"type":"status","run":{"status":"succeeded"}}delta — a status transition is the one message that has no reason to repeat the id already in the topic name — failsdecodeFromString<TaskEvent>, and:1051discards it:No log, no state change. The terminal frame vanishes and the 10s watchdog is the only thing that eventually notices. Default
idto0Llike everything else — the follow already knows which run it is asked for.Pair it with logging: this is the only decode failure in the new code that is silent.
probe, the snapshot path,sub-err, the dropped-frame paths all log. A swallowed frame that changes what the screen believes should not be the quiet one.Should fix
S1 —
runCatchinginprobe()swallowsCancellationException.:963. Every frame callsarmWatchdog()→silence?.cancel()(:996), cancelling an in-flight probe;taskRun'swithContextthrows,runCatchingcatches it,:967counts it as a network failure. It can't reach the threshold today — the collector zeroesprobeFailuresat:1016before the cancel lands — but the budget is left dirty and the pattern is a trap for the next edit..onFailure { if (it is CancellationException) throw it }.S2 — a slow probe response can overwrite newer live state.
probe()→showRun(run)(:989), no sequencing check. The probe fires after 10s of silence; a frame arriving during the round-trip updates the feed, then the older snapshot replaces it —lineswholesale, andrunStatuscan regress (live "cancelling" → stale "running"). Self-correcting, and terminal is safe becauseendRuncancels the probe's parent, so it's a flicker not a wedge. SkippingshowRunwhen a frame landed after the probe started closes it.S3 —
runOnSuccessparks a composable's lambda in the ViewModel for the life of the run.:271, assigned at:1310from the closureSchemaScreenrebuilds on every recomposition, invoked minutes later at:1157. It capturesonNavigateSub. Put the interpolatedsuccessPageinSchemaUiStateas a one-shot event and let the screen navigate; the ViewModel shouldn't hold nav callbacks.S4 — a refused refresh topic dies silently.
startRefreshTriggers:382collectspolicy.gate(topic)and callsrefetchAll()on any event,LiveEvent.Rejectedincluded — so a rejection fires one refetch and the page then never auto-refreshes again, with nothing said. The log-follow path at:462at least documents the choice ("costs freshness, not correctness"); a page's refresh trigger is a stronger claim to lose quietly. Now thatRejectedexists, both sites should discriminate on it.S5 — per-frame Job churn.
armWatchdog()cancels a coroutine and launches a fresh one for every frame (:1018) — one allocation per log line on Main. The_statecopy per line dominates, so this isn't the bottleneck, but alastFrameAtMstimestamp checked by one long-lived loop is the same behavior with none of it.S6 — a defaulted
keyeats the legacy text fallback. WithProgressEntity.keydefaulted, a legacy frame carrying a malformedprogressobject now decodes instead of throwing, sorunAction's "fall back to the rawmsgas a plain text line" (:851) no longer runs — the entity becomes an empty-key row and the message text is lost. The M1 trade is still right; it's just no longer true that a bad payload always surfaces as text.Nits
UpdateProgressCard.kt:141vs:171. The resources comment argues the tail is correct because the newest rows are the ones still moving; services gettake()with "same budget as resources". And the overflow labels disagree about which end was cut — "+ N more" vs "+ N earlier".withheldrenders raw wire field names ("entities", "lines") straight intoNot shown for your role:. A small map to operator-facing words would finish the thought the field exists for.UpdateProgressCard.kt:99) has no trailingSpacer, unlike every sibling block in the card.TaskRunfields are decoded and never read:module,action,target,title,origin,actor,result,steps. Post-M1 they're all defaulted so they can no longer break a decode, but the class KDoc still describes a richer screen than exists.Tests
SubStateTestandTaskRunTestcover the pure logic properly — the re-snapshot merge test and the all-cached rollup test are the right shapes, and "one degraded entity or step cannot take the whole snapshot down" pins exactly the invariant B3 still violates one field short of.The gap is unchanged and now larger:
followRunholdsprobe(),armWatchdog(), the failure budget, the foreground check, the form-run success routing and theendRunbanner routing, and none of it is exercised. B1, B3, S1 and S2 all live in that coroutine. No ViewModel test exists anywhere in the repo, so this is a new fixture rather than a missing one — butrunTestplus a fakeApiClient/LiveClientwould have caught at least three of the findings above, and this is the file where a silent wrong answer costs the most.Housekeeping
versionName0.30.0 → 0.31.0 is correct per CLAUDE.md (feature, pre-1.0 minor). Commit messages are Conventional. No bump in the last commit — fine while 0.31.0 is unreleased on the branch, flagging so it's a decision.One editorial note: a large share of these +1748 lines is KDoc, several blocks past 15 lines. It's genuinely why rather than what, which is the good kind — but
Task.ktandfollowRunnow carry design-doc paragraphs that will drift faster than the code under them. The last round already produced one instance (a doc assertingwithheldhandling that didn't exist yet); B3 is the next, since the "every field defaulted so one omission can't kill the snapshot" argument is written down inRunStep's KDoc whileTaskRun.idquietly doesn't follow it.N1–N6 addressed in
eded824.probe()rethrowsCancellationException(.onFailure { if (it is CancellationException) throw it }) instead of counting its own cancellation as a network failure. The budget stays honest whatever the coroutine tree does around it.lastFrameAtMssuggestion, as written: the collector writes a timestamp (and bumps the counter N2 reads), and one long-lived loop sleeps until a probe could be due, re-baselining on whatever the collector wrote. No Job per frame; the loop wakes at most once perFOLLOW_SILENCE_MSno matter how fast apt prints. A side effect worth naming: a frame no longer cancels an in-flight probe — N2's staleness check is what defuses it instead, which is also what makes N1's dirty-budget path unreachable rather than merely rare.Stringfor the run's life and surfaces as one-shotSchemaUiState.openPage; the screen navigates in aLaunchedEffectand acknowledges viapageOpened()before navigating, so a recomposition on the way out cannot fire it twice.submitFormlost its callback parameter entirely — the immediate (non-run) path goes through the same event.+ N earlier, same budget, same end, same wording as resources. The comment now argues it once for both.keydemotes back to its rawmsgon the legacy stream (ProgressEvent.textUnlessKeyed()): the defaulted key exists for the snapshot bulk decode, and identity is what makes an entity an entity. Tested — the demotion and that a keyed entity passes through untouched.Still open, deliberately:
./gradlew testDebugUnitTestgreen (76),assembleDebuggreen. Still not exercised against a live server —module_updatesremains the companion PR.Review at
eded824../gradlew testDebugUnitTestgreen.fix(schema): the probe's blind spots, and nav state out of the ViewModelcloses S1, S2, S3, S5, S6 and the head/tail nit — cleanly, and in the right places. It does not touch B1, B2, B3 or S4, which were the three blocking items and the silent-refresh one. Re-requesting changes on those; they are unchanged, not re-argued below.Closed well
.onFailure { if (it is CancellationException) throw it }(:983) — and it sits before the failure accounting, so a cancelled probe no longer dirties the budget.framessnapshot (:967, checked at:985) is a better fix than the timestamp I suggested: it keys on "did the collector actually process something", not on clock arithmetic.openPageas one-shot state, acknowledged viapageOpened()beforeonNavigateSub(SchemaScreen.kt:125-131), and theLaunchedEffectis above the loading/error early-returns so it can't be composed away. Both details are the ones that usually get this pattern wrong.SystemClock.elapsedRealtime()— monotonic, right clock, no per-frame allocation.textUnlessKeyed()scoped to the legacy stream only, leaving the snapshot's keyless-entity trade intact. Exactly the right seam, and the test pins both directions.New
N1 — a frame arriving during a probe doubles the cadence instead of resetting it.
:985returnstruefor the stale case, and the loop then unconditionally runsbaseline = elapsedRealtime()/delayMs = min(delayMs * 2, …)(:1045-1046). Becausebaselineis now ahead oflastFrameAtMs, thelastFrameAtMs > baselinereset at:1026can't fire on the next pass either. So the one case that proves the channel is alive is the case that backs the probe off — the opposite of the stated rule ("a frame proves the channel delivers: the probe's cadence and failure budget start over").probeFailuresdoes get reset by the collector, so only the cadence is wrong. Distinguish stale from healthy-still-running in the return, or setbaseline = lastFrameAtMs; delayMs = FOLLOW_SILENCE_MSon the stale path.N2 — navigating away doesn't orphan the follow, it hides it — and the KDoc says otherwise.
:935claims "navigating to another page orphans the follow".MainScaffold.kt:335puts every module page on onecomposable("module/{name}?path={path}")route, so each page is its ownNavBackStackEntryand its ownViewModelStore; pushing a sub-page leaves the previous entry — and itsSchemaViewModel— alive on the back stack.So the follow keeps running invisibly: the
core:task:<id>subscription stays open and the probe loop keeps firing every 10–30s, because theProcessLifecycleOwnercheck at:1038only skips when the whole app is backgrounded, not when the page is off-screen. "Stop following" is unreachable from another page, and several stacked module pages each with a run means several concurrent loops. The doc is wrong in the safe direction, but the behavior is a real background cost with no off switch.viewModelScope+ aLocalLifecycleOwner-driven pause, or simply gating the probe on the page being resumed, matches what theProcessLifecycleOwnercheck was already reaching for.N3 — a deferred
openPagefires on return, not on success. Follows from N2: form on page A starts a run, operator navigates to B, the run succeeds while A is off-screen,finishRunsetsopenPage(:1194), and nothing consumes it until A is composed again — at which point coming back to A immediately bounces the operator to the successPage. Possibly what you want; it is a behavior change from "navigate on success" and worth being a decision rather than a consequence.Still open from the last review
runAction:835andsubmitForm:1262both guard onrunningActiononly; a form submit setssubmittingForm, so every action stays live for the whole request (minutes on a multipart upload) and the secondrunIdresponse cancels the first follow.progressTopicrun now freezes the forms too (runInFlight = state.runningAction != null,SchemaScreen.kt:282), and the escape hatch is gated onstate.runId != null(:255), which is null for the legacy path.TaskRun.id(Task.kt:28) is still the one required field; astatusdelta without it fails the wholeTaskEventdecode and is dropped silently at:1094.:428) fires onerefetchAll()and the page then never auto-refreshes again, with nothing logged.Plus the ViewModel test gap. N1 is the third finding in a row inside the probe loop, and the loop is still the only part of
followRunwith no test around it.ba5fe5fanswers the open items from the review ateded824../gradlew testDebugUnitTestgreen (83).B1 — the lock is one derived flag now:
runLocked = runningAction != null || submittingForm != null, tested byrunActionandsubmitFormin the model and mirrored byActionsRowandFormCardon the screen. A form submit holds it from the moment it POSTs. The confirm dialog funnels intorunAction's guard, so a dialog opened before something else claimed the lock confirms into the guard, not the hole.B2 — "Stop following" is gated on
runningAction != null(runId or not),endRuncancels the legacyprogressJobtoo, and arunEpochcounter (bumped byendRunand page turns, captured at press) keeps a POST response that lands after "stop following" from resurrecting the lock — it refetches instead of following.B3 —
TaskRun.iddefaulted to0, with the reasoning in its KDoc (a status delta has no reason to repeat the id already in the topic name). TheTaskEventdecode failure now logs — it was the only silent one. Test added: an id-less{"type":"status"}delta decodes and reads terminal.S4 —
LiveEvent.Rejectedon a refresh topic is recorded and logged instead of firing a refetch. Recorded in a set of topic names, not a count:RefreshPolicy.gateisflatMapLatestover the mode, so a mode change re-collects the source and the same topic can be refused again (caught by a cavecrew pass before push). The interval fallback stops trusting a connected socket once every topic is refused, and a topic that later delivers is removed from the set.N1 — the cadence is extracted into
ProbeCadence, pure and tested. The loop comparesframesbefore/after the probe and skipsprobed()on the stale path, so the nextwaitForre-baselines on the frame — base delay, no doubling.ProbeCadenceTestpins the regression case by name.N2 — the screen reports its
NavBackStackEntrylifecycle viasetPageVisible(DisposableEffect + LifecycleEventObserver,onDispose→ false); the probe's foreground check requires it.followRun's KDoc now says what actually happens on navigation: the entry and its subscription stay alive on the back stack so the outcome is current on return, and only the probe sleeps.N3 — decided: a run that ends while the page is off-screen drops its deferred
successPage. Navigation that fires the moment the operator comes back is a bounce they never asked for; the refreshed page and the form banner say what happened. Written down at the site infinishRun.Deliberately not done this round:
probed()stays outside the foreground check — a skipped probe must still advance the baseline or the loop spins hot while the page is off-screen; now commented at the site. The ViewModel test fixture (fakeApiClient/LiveClient+runTest) remains open — it needs both classes behind interfaces and is a refactor I'd rather not fold into a review-fix commit;ProbeCadencemoves the probe's decision logic out of the coroutine the same waySubStatedid for the reconnect story.Review at
ba5fe5f../gradlew testDebugUnitTestgreen.B1, B2, B3, N1, N2, N3 are closed, and
ProbeCadence+ProbeCadenceTestis the right move — extracting the rule so it can be pinned, including a regression test that encodes the old inverted behavior, is exactly what was missing.endRuncancellingprogressJobcloses the legacy leak.But the epoch — the new mechanism this commit introduces to make the new exit safe — is wired into one branch out of four, and the new exit is what makes the race reachable. So this round trades two closed bugs for one opened one.
Blocking
B4 — the run epoch guards one response branch out of four, and this commit made the race reachable
runAction:955-999.epochis captured at press time and compared only in therunIdbranch. The other three write run state unconditionally:Repro — every step is possible only because this commit gated the exit on
runningAction:runningAction = "A", epochE.runId, so unreachable for an action still POSTing).endRunreleases the lock, epoch →E+1.runningAction = "B".runningAction = null. B's lock is released while B is still in flight. B's spinner clears, B'sactionErroris overwritten by A's, B'sprogressJobis cancelled (it's a field now, and it belongs to B). A third writer can claim. If B's response later carries arunIdwith B's own epoch it will happily follow — after the screen has already said B was over.submitFormcaptures no epoch at all. That path is unreachable today only because the exit isn't shown whilesubmittingFormis set — one bug masked by another (B5).The fix isn't another
if: it's making the epoch, not the branch, decide whether a response still owns the state. Oneif (runEpoch != epoch) return@fold(plusrefetchAll) at the top of bothfoldarms, in both writers.B5 — the lock and the exit are gated on different predicates
runningAction != null || submittingForm != null(SchemaViewModel:890, mirroredSchemaScreen.kt:149).runId != null || runningAction != null(SchemaScreen.kt:289).The gap is
submittingForm != nullwith norunningAction: the whole page — every action and every form — locked, with no way out. Widening the lock to cover the submit window was the correct B1 fix; the exit didn't follow.Uploads use the shared client (connect 10s / read 30s, no
writeTimeoutoverride), so a stalled request self-clears in tens of seconds. A legitimately slow multi-minute multipart upload does not — it holds the entire page with no cancel, which is the same wedge B2 was about, relocated. Gate the exit onrunLockedand letstopFollowingabandon an in-flight submit the way it now abandons an in-flight action POST; the epoch already does that work once B4 is fixed.Should fix
S7 —
pageVisibleconflates "another page is on top" with "the app isn't resumed this second".SchemaScreen.kt:135-146derives it fromisAtLeast(RESUMED). The notification shade, a permission dialog, a transient pause all drop the entry below RESUMED.finishRun:1312then discards the form'ssuccessPage— not defers, discards — so a run that terminates during a shade pull silently loses its navigation for good. The intent ("the operator is elsewhere in the app") is the entry falling below STARTED, or a comparison against the nav controller's current entry. The probe pause shares the trigger but there it only costs a delay.S8 — the S4 fix is half applied: a rejected log follow still counts as push coverage.
:495computespushCovered = refresh.topics.size > rejectedRefreshTopics.size || hasFollow, buthasFollowis a static property of the page's blocks and the log-follow collector still swallows the rejection at:537(is LiveEvent.Rejected -> Unit). A page whose only push coverage is a follow the server refuses keepspushCovered = trueand never falls back to the interval — the exact failure just fixed one branch up.S9 — "one lock" is defined twice.
SchemaViewModel:890andSchemaScreen.kt:149are the same expression, in a commit titled one lock for every writer. Put it onSchemaUiStateas a derivedval runLockedand have the model and the screen read the same thing. Otherwise the next field that joins the lock has two sites to update and one will be missed — which is precisely this PR's history: H1 → B1 → B4.S10 — the mutual exclusion depends on
Dispatchers.Main.immediate. Both guards areif (runLocked) returnoutside the coroutine; the write that closes the lock is insideviewModelScope.launch. It holds only becauseviewModelScopeisMain.immediateand nothing suspends before the_state.update, so the write lands synchronously with the press. Add one suspending call above it and two taps double-claim. Set the lock beforelaunchand the invariant stops being load-bearing on a dispatcher detail.S11 — one staleness rule, two implementations.
probe()snapshotsframesand discards a stale response (:1061,:1072); the loop snapshotsframesagain to skipcadence.probed()(:1129,:1131). Consistent today only because nothing suspends between them. A tri-state return fromprobe()(ended / stale / silent) collapses it — and is whatProbeCadence's own KDoc already describes.S12 — the cadence isn't reset when the page comes back. After time off-screen the backoff sits at
RUN_PROBE_MAX_MSand the skipped-probe path keeps pushing the baseline forward, so returning to a page whose channel is dead shows stale run state for up to 30s.pageVisibleflipping true is the natural reset point.Nits
ProbeCadencehas no Compose or Android dependency but lives inui.schema. It's model logic;data/or afollow/package says what it is.waitFor()reads as a query and mutatesbaseline/delayMs. Documented, but a name liketick()stops the next reader from calling it twice for a peek.TaskRun.id = 0makes "absent" and "zero" the same value. Nothing readsrun.idtoday; aretrycapability keyed on the linked run would.Out of scope, but it's in these files
SchemaScreen.kt:167—ErrorRetry(state.error!!, onRetry = { viewModel.start(module, subPath) }).start()returns early whenstarted && this.module == module && this.subPath == subPath(:317), which is always true by the time an error is on screen. The retry button does nothing. Pre-existing, not this PR, but it's a dead control in the file being rewritten.On the pattern
Six rounds, and the recurring finding is one shape: run state is four single-valued fields plus three plain
vars (runFromForm,runSuccessPage,runEpoch,pageVisible) coordinated by hand at each site that touches them. Every round fixes the sites it can see and the next writer reopens it — H1 → B1 → B4 is the same bug three times, each time in a place the previous fix didn't reach.That converges when the state becomes one thing: a
RunClaim(or a sealedRunState) holding label, epoch, origin, deferred page and follow job, with a singleclaim()/release()pair that every writer goes through and one derivedlockedthe screen reads. Then B4 is impossible by construction rather than by remembering to check, and the tests that don't exist yet have something with a surface to test.ProbeCadenceis proof the approach works here — it's the one piece of this machinery that stopped producing findings the moment it was extracted and pinned.Round 7 addressed in
2771957../gradlew testDebugUnitTestandassembleDebuggreen.Blocking
B4 — the epoch, not the branch, decides ownership. Both
foldarms inrunActionandsubmitFormnow open with the epoch check. A response whose lock was released mid-flight touches no run state at all — not the lock, not the banners, notprogressJob(a new claimant may own it) — it refetches and walks away. The repro chain (stop-following mid-POST → new claimant → old response lands) is dead at step 4: A's 500 compares its captured epoch, sees the bump, and leaves B's claim alone.B5 — the lock and the exit are the same predicate now.
endRunreleases the submit half too (submittingForm/uploadPercent/uploading), and the escape-hatch row is gated onstate.runLocked— the same fact the buttons disable on. An in-flight submit shows "Stop waiting" (nothing to follow yet); the epoch bump tells its response nobody is waiting. The request itself is not cancelled — same treatment as an abandoned action POST, noted in the KDoc.Should fix
STARTED, notRESUMED. A shade pull or permission dialog only pauses the entry; back-stacked entries fall belowSTARTED.finishRunno longer discards a successPage over a notification.rejectedFollows(block indexes):Rejectedrecords + logs, any delivered event re-arms, andpushCoveredcounts only follows that weren't refused. Both halves of the S4 fix now exist.runLockedis derived once, onSchemaUiState. Model guards, screen disabled-states and the exit all read it; there is no second definition site left to miss.launch. The mutual exclusion no longer rides onMain.immediate; theclientOrNull == nullpath releases the claim it just took.probe()returnsENDED / STALE / SILENT. The staleness rule lives inprobe()alone; the loop acts on the verdict and no longer re-derives it fromframes.pageOnScreenis aStateFlow; the loop sleeps inwithTimeoutOrNull(wait) { pageOnScreen.dropWhile { it }.first { it } }, so it wakes on the off-screen→on-screen edge whichever state the sleep started in, andcadence.reset(lastFrameAtMs)forgets the backoff. A dead channel is probed immediately on return; a recent frame keeps its quiet window.ProbeCadenceTestpins both halves.Nits
ProbeCadencemoved todata/follow/— no Compose or Android dependency, and the package now says what it is.waitFor→tick, with the mutation named in its KDoc.TaskRun.id = 0left as-is per your note — nothing reads it today; the day aretrycapability keys on it, it becomes nullable.Out of scope, taken anyway
The dead retry button:
retry()clearsstartedbefore re-enteringstart(), and the error screen calls it instead ofstart()directly.On the pattern
Not the full
RunClaimextraction this round — but the two mechanisms it would buy are now singular: one lock (SchemaUiState.runLocked, derived), one ownership rule (the epoch, checked at the top of every writer's response, not per branch). B4-by-construction is what's left of the argument, and I'd rather take that refactor on a green branch than fold it into round 7.🤖 Generated with Claude Code
Review at
2771957../gradlew testDebugUnitTestgreen.B5, S7–S12 and the dead retry button are all properly closed, and several of them better than I suggested:
ProbeResultcollapses the duplicated staleness rule into one verdict the loop merely acts on;runLockedonSchemaUiStatemakes the lock genuinely one fact; thedropWhile { it }.first { it }edge detector is correct in both start states, which is the part that's easy to get wrong. STARTED vs RESUMED is the right line for "another page is on top" — a back-stack entry under another page drops to CREATED, while the visible one keeps STARTED under a shade.B4 as I stated it is closed. The principle I stated under it — make the epoch, not the branch, decide ownership — is not: it moved from "one branch out of four" to "two response handlers out of five writers".
Blocking
B6 — three writers still touch run state with no epoch
(a) The multipart progress callback.
:1525:stopFollowing()releases the lock but explicitly does not cancel the request — so an abandoned upload keeps streaming and keeps writinguploadPercent. Concretely:submittingForm = "A", upload running.endRunclearssubmittingForm/uploading/uploadPercent, bumps the epoch. A's upload carries on.submittingForm = "B".FormCard.kt:337renderssubmitting == block.submit.label && uploadPercent != null→ form B displays form A's upload percentage, and the two callbacks then fight over the field.The epoch is already captured in that scope; the callback is the one place it wasn't threaded through.
(b)
cancelRun(),:1326-1343. CapturesrunIdat tap time, writesactionError/runStatus/runCancellableat response time, checks nothing in between. Tap Cancel → tap Stop following → press another action → the refusal lands and overwrites the new claimant's banner, orrunStatus = CANCELLINGis written onto run state that no longer belongs to the run that was cancelled.(c) Releases don't bump the epoch. Only
start:365andendRun:1428bump. These release the lock without one::969,:1029,:1046,:1059(action),:1494,:1548,:1557,:1603(submit). So after a normal completion the next claimant captures the same epoch its predecessor had. That is safe today only because no other writer is in flight at those moments — precisely the assumption the epoch was introduced to stop relying on, and precisely the reasoning that made B4 look safe last round.The fix is the same one as last round, and it is now the only thing left generating findings: one
claim()/release()pair that every writer goes through, with the bump insiderelease()and the epoch handed to the claimant. Then "is this still ours" is one comparison in one place, and (a)/(b)/(c) cannot be forgotten individually because there is nowhere to forget them.Should fix
S13 — page toggling makes a probe due immediately, every time.
cadence.reset(lastFrameAtMs)fires on every off→on edge (:1193); with a dead channel the nexttickreturns<= 0and probes at once. Navigating A→B→A→B is entirely user-driven, so the endpoint gets hit on each return. The reset is right — it just needs a floor (no probe within, say,FOLLOW_SILENCE_MSof the last one) so a fidgety back button can't drive the request rate.S14 —
withTimeoutOrNull { … } != nullon a block that returnsBoolean.:1189-1193. It works becausefirst { it }can only yieldtrue, sofalseis unreachable — but the expression compares aBoolean?against null, where "returned false" and "timed out" would be indistinguishable the moment that changes.== trueis the same length and can't rot.S15 — the successPage discard is still an instantaneous test.
finishRun:1390readspageOnScreen.valueat the exact millisecond the terminal frame lands. STARTED fixed the shade case, but a two-second app switch at the wrong second still discards the form's navigation permanently — it is dropped, not deferred. The stated intent ("the operator moved on, minutes ago") is a duration; the code is an instant. A last-on-screen timestamp with a short grace matches what the comment argues for.Nits
import cloud.imhof.opsdeck.data.follow.ProbeCadencesits betweendata.model.*anddata.net.*(:39) — out of order. No ktlint/detekt/spotless in the build, so nothing catches it; worth noting that this PR is now ~1900 lines with no formatter gate.stopFollowing()now covers three things: stop following a run, stop waiting on a POST, abandon an upload. The button already switches label; the method name is the one that didn't.ProbeCadencemoving todata/follow/is right. It is also still the only piece of the follow machinery with a test.Standing
The ViewModel test gap, unchanged across five rounds.
ProbeCadenceTestgrew again and is genuinely good — thecoming back on screen forgets the backofftest even pins the "reset is not a probe trigger" half, which is the subtle one. Everything the tests cover has stopped producing findings. Everything they don't (runAction,submitForm,endRun,cancelRun, the epoch) has produced one per round, in a new place each time: H1 → B1 → B4 → B6. That is not four bugs, it is one design being patched at the call sites four times, and B6(c) says the next one is already sitting there waiting for a second writer to exist.Round 8 pushed as
1ca7b52../gradlew assembleDebugand./gradlew testDebugUnitTestgreen (90 tests).B6 — the claim is now the thing, not a convention over four fields
Taken as stated: the fix is not more checks, it is making ownership one fact. New
RunClaimindata/follow/:claim()hands the writer a token;release()retires every outstanding token unconditionally — the epoch bump is inside release, so a completion path structurally cannot open the lock and forget the bump;owns(token)is the single comparison every asynchronous continuation presents its token to.releaseRun()in the ViewModel is now the only place the lock opens. Every completion path goes through it: 400, transport failure, plain 2xx, client-null bailout, and the legacy stream'sdoneline — that last one closes (c): a POST response landing after the stream already declared the outcome now finds its token dead and just refetches.uploadPercent. The epoch is@Volatilefor exactly this reader: the callback outlives an abandoned claim by design (abandoning does not cancel the request) and runs on an IO thread, so the release must be visible there or the abandoned upload keeps painting its percentage over the next claimant's.cancelRuncapturesclaim.tokenat tap time and checks it when the verdict lands — a refusal arriving after stop-following + a new claim writes nothing.submittingFormforrunningActionin one_state.update— no frame where neither is set and the lock reads open.RunClaimTestpins the lifecycle, including the collision that was (c): claim A, release, claim B — A's token must not own B's state.Should-fix
ProbeCadencegained the floor —tick()is never due withinsilenceMsof the last probe, however oftenreset()re-arms the schedule. The reset stays a re-arm, not a probe trigger; the toggle case is tested (toggling the page cannot drive the probe rate above the floor), and the existingcoming back on screentest now asserts "due one base window after the last probe" instead of "due immediately".== true.leftScreenAtMsis stamped on the visible→invisible edge andfinishRungrants a 5s grace (SUCCESS_NAV_GRACE_MS), so a shade pull or a two-second app switch at the wrong moment no longer discards the successPage for good. Kept deliberately short: a success landing off-screen inside the window still navigates when the page is next composed, so the grace is also the bounded bounce window — the constant's doc says so.Nits
stopFollowing→abandonRun, with the three waits it ends named in the KDoc.data.followimports sorted where they belong.Standing
The pure surface grew again instead of the coroutine getting a fixture:
RunClaimjoinsProbeCadenceandSubStateas extracted, pinned machinery — the epoch, its origin routing and the deferred successPage are now testable and tested, which is where B6(a)/(b)/(c) lived.followRun's coroutine wiring itself remains untested, same trade as every round.No
versionNamebump — 0.31.0 is still unreleased on this branch.Review at
1ca7b52../gradlew testDebugUnitTestgreen.B6(a), B6(b), B6(c), S13, S14 and both nits are closed, and
RunClaimwith the bump insiderelease()is the right structural move — B6(c) is now impossible rather than remembered.cancelRun's bystander token is exactly right and is tested. The probe floor is tested with the numbers that matter.Two new defects, both created by this commit, and the claim abstraction stops one step short of being the thing it is named after.
Blocking
B7 — the success-nav grace is checked at the wrong end
finishRun:1404decides whether to setopenPagefrom the page's visibility at the moment the terminal frame lands.openPageis consumed byLaunchedEffect(openPage)when the screen composes. A NavBackStackEntry below the top is not composed, so the flag sits in state until the operator navigates back.leftScreenAtMsstamped.SUCCESS_NAV_GRACE_MS, soopenPageis set.LaunchedEffectfires and bounces them to the successPage.That is verbatim the outcome the KDoc argues against ("a page left minutes ago must not park a bounce for whenever they come back"). The grace bounds when the flag may be set; nothing bounds when it fires. Before this commit the test was instantaneous, so an off-screen success simply dropped the page — no parked bounce existed. The grace introduced it.
Worse on the other path: the non-run submit at
:1620-1624setsopenPagewith no visibility check at all, and that path can be the slower of the two (a multipart upload runs for minutes before its 2xx). Page away mid-upload and the bounce is guaranteed, whenever you come back.One check at consumption covers both and is the only place that can know how long the request waited: stamp
openPagewhen it is set, and have the effect (orpageOpened) discard one older than the grace.B8 — releasing on the legacy stream's "done" retires the token of the POST that started it
:1004. TheprogressTopicstream and the HTTP response are independent transports. When the stream's "done" arrives first,releaseRun(null)bumps the epoch — and the action's own response then failsclaim.owns(token)at:1024, hits the disowned branch, and returns afterrefetchAll().If that response was a 4xx, its error is silently swallowed: the operator sees the stream's "done" and no failure. Before this commit the done path cleared
runningActionwithout a bump, so the 4xx branch still reported.The ordering is narrow — the legacy contract is "respond 2xx, then stream" — but nothing enforces it, and a fast module that finishes before its own response unwinds hits it. This is a new way to lose an error, created by the same change that closed B6(c).
Should fix
S16 —
releaseRunis documented as the only gate and doesn't check the token. Six call sites::977and:1519trivially own (immediately after claiming),:1033/:1045/:1056and:1580/:1621/:1632sit inside anownscheck,:1438is the deliberate unconditional abandon — and:1004is neither, which is B8.releaseRun(token, error)that no-ops when!claim.owns(token)makes the gate actually gate, and B8 stops being something to reason about.S17 —
RunClaimcannot distinguish "held" from "free", so it is not yet the mutual exclusion.claim()sets the payload and returns the current epoch without bumping, so twoclaim()calls with no release between them hand out the same, still-valid token;owns()also returns true for a token read when nobody holds anything. The class is an epoch counter with two payload fields. Actual exclusion still lives inSchemaUiState.runLocked, consulted separately by the two writers and the screen — a second mechanism in a second place, which is the exact shape that produced H1 → B1 → B4 → B6.Related: both writers still take the lock and the claim as two statements (
_state.update { runningAction = … }thenclaim.claim()), correct only because nothing suspends between them — the same reasoning S10 was raised about. Aheldflag,claim()rejecting when held, andlockedderived from the claim would put all of it in one object.S18 —
probed()records probes that never ran.:1222is deliberately outside the foreground check so a skipped iteration still advances the baseline — but it now also stampsprobedAtMs, which the new floor measures from and whose KDoc says "when the last probe ran". Every skipped tick while the page is off-screen pushes the floor forward, so the first real probe after returning is delayed by up tosilenceMsbeyond what the reset intended. Split it: askipped()that movesbaseline/delayMsand leavesprobedAtMsalone.S19 —
@Volatilecoversepochand nothing else. The class doc says the token is read from the upload callback's IO thread;fromFormandsuccessPageare plain fields on the same object. Correct today because IO only callsowns()— but nothing in the type says so, and the next IO-side reader will findsuccessPagesitting right there. Either swap an immutable claim through anAtomicReference, or make the cross-thread surface a single accessor and keep the rest private.Nits
RunClaimTestcovers release-between-claims and the bystander well, but not the invariant the ViewModel actually depends on: two claims with no release. That's the one S17 says the class can't defend — worth either a test documenting the current behavior or an assertion inclaim().abandonRunrename is clean — no stalestopFollowingreferences anywhere inapp/src.Standing
ProbeCadenceandRunClaimare both extracted and pinned now, and neither produced a finding this round that wasn't about how the ViewModel calls them (S16, S18). B7 and B8 are both in the glue — the untested part, five rounds running. Every finding left is now inrunAction/submitForm/finishRun/endRun, and those four functions have never had a test.Pushed
3498205../gradlew testDebugUnitTestgreen — 103 tests.B7 — the grace moved to the consuming end.
requestPagestamps when the navigation was asked for;pageOpenedreturns the page only if the stamp is still insideSUCCESS_NAV_GRACE_MS, and spends the request either way. The screen'sLaunchedEffectnavigates to whatpageOpenedhands back rather than to what it read out of state, so a request parked while the entry sat below another one is discarded when the page finally composes.finishRunno longer tests visibility at all, which also fixes the other half: the non-run submit at the end ofsubmitFormgoes through the samerequestPage, so a multipart upload that answers after minutes is bounded by the same rule.leftScreenAtMsis gone — it had no other reader.B8 —
LegacyEnding. The stream's "done" and the POST's response are independent transports, so neither retires the claim on its own: whichever lands second releases. The 4xx branch therefore still owns its token when the stream finished first, and reports. If the response never lands the claim stays held and "Stop waiting" is the exit — deliberately not a timeout, since reporting an outcome the app does not have is what this PR exists to stop. Both orders are tested.S16 —
releaseRun(token, error)no-ops when the token no longer owns; the unconditional overload is now onlyendRun's deliberate abandon. Every call site that sits under a claim went through it, including the two that trivially own.S17 —
claim()returnsnullwhile the state is held, and both writers doclaim.claim(...) ?: returnbefore writing the state it guards. The_state.value.runLockedguards are gone: the exclusion is one act in one object, not a flag checked next to a claim taken.SchemaUiState.runLockedstays as what the screen disables on, with its doc saying so. Tested: a second claim with no release between is refused and does not disturb the holder, andownsis false when nobody holds.S18 —
skipped()split out ofprobed(). It movesbaselineanddelayMs; only a probe that ran stampsprobedAtMs. Regression test: probe at 10s, skipped tick at 30s, return at 32s — due at once, where stamping the skip held it off another 8s.S19 —
RunClaimholds one immutableHeldin anAtomicReference, sofromFormandsuccessPageare as visible to the IO-side reader as the epoch was. Writes are still Main-only, hence no CAS loop, and that is what the class doc says now.Nit: still no ktlint/detekt gate — left out deliberately, since it is a repo-wide tooling change rather than part of this branch.
🤖 Generated with Claude Code
Review at
3498205../gradlew testDebugUnitTestgreen.B7, B8, S16, S17, S18, S19 and the
RunClaimTestgap are all closed. First round with nothing on the blocking list. Not approving yet — S21 below is a real weakness in the primitive everything else now leans on, and S20 is a dependency on behavior nobody has verified — but neither is a demonstrated defect, and I am not going to invent one to keep the label.I traced the two fixes rather than taking the commit message for them:
B7.
requestPagestamps,pageOpened()decides and spends either way, screen doespageOpened()?.let { onNavigateSub(it) }. The mechanism is right for the case it was raised about: a NavBackStackEntry under another page leaves composition, so theLaunchedEffectis cancelled and re-runs on return — where the request is found stale and spent without navigating. AndrequestPagenow covers the plain-submit path too, which was the half with no check at all.B8. Walked all four orderings. Respond-then-stream:
responded()false → no release →streamDone()true → release. Stream-then-respond:streamDone()false → no release → 4xx reachesreleaseRun(token, msg)and reports, 2xx takes the newstreamEndedbranch and releases without a second refetch.LegacyEndingis two booleans and a rendezvous, which is the right size for the problem, and the test names the inverted order as the one that matters.releaseRun(token, error)as a second overload is the correct shape for S16 — the abandon keeps its unconditional path, everything that completes under a claim goes through the checked one.claim()returningInt?makes the guard and the claim one act, andowns()requiringheldcloses the "token read out of a free claim" hole thatnobody holding is nobody owningnow pins.Should fix
S21 —
RunClaimpresents itself as the mutual exclusion, uses anAtomicReference, and implements read-modify-write non-atomically.Correct today — every claim and release is on Main, as the KDoc says. But the class doc now opens with "[claim] is the mutual exclusion itself, not merely its bookkeeping", and the container is an atomic one precisely because another thread reads it. That combination invites exactly the assumption the implementation does not honor: two concurrent
claim()calls both pass, two concurrentrelease()calls lose an epoch.compareAndSetin a small loop (orupdateAndGet) costs nothing here and makes the type mean what it says.RunClaimTestis single-threaded, so nothing pins the Main-only assumption either.This is the primitive that four rounds of findings converged on. It should be the one place in this PR where a reader does not have to check the surrounding code to know it is safe.
S20 — the grace now depends on when Compose chooses to recompose, and one of the two "operator left" cases is not the one it was tested against.
pageOnScreenno longer participates in the navigation decision at all — it is read only by the probe loop now. Freshness at consumption is the whole rule, and consumption is aLaunchedEffectfiring on recomposition. That splits cleanly for the back-stack case (composition disposed → consumed on return → stale → dropped, which is B7 fixed). It does not split cleanly for the app being backgrounded: composition is retained, and whether the effect launches promptly or waits for the window to come back depends on the recomposer's frame clock — not on anything this code states.If it fires promptly, a success landing during a long background gets navigated immediately and the operator returns to a page they never asked for; if it waits, the grace expires and the request is dropped. Both are defensible, but the code picks neither on purpose.
repeatOnLifecycle(STARTED)around the consumption, or an explicitpageOnScreencheck insidepageOpened(), makes the choice visible instead of inherited.SuccessNavTestcoversnavStillWantedwell; it cannot cover this, because this is about when it is called.S22 — a transport failure after the stream already said "done" reports a failure the app does not have.
onFailurecallsending.responded(), ignores the answer, and writesactionError = e.message. So: module finishes, stream says "done",refetchAll()runs, then the POST times out at 30s → the operator gets "timeout" over an action whose work visibly completed.LegacyEnding's own doc says "Reporting an outcome the app does not have yet is what the whole task-run change exists to stop" — this is the same sentence with the sign flipped. Whenresponded()returns true, the ending is known; the transport error belongs in the feed, not in the verdict banner.S23 —
navStillWantedis the only extracted rule that did not move. It is aninternal funsitting inSchemaViewModel.ktwhileProbeCadence,RunClaimandLegacyEndingall live indata/follow/. Same treatment, same place.Nits
claim()is a silentreturn. The screen disables the buttons, so the only way in is the confirm dialog opened before something else took the state — and then Confirm closes the dialog and nothing happens, with no explanation. The refusal is now correct; it is still invisible.LegacyEndingis allocated for every action, including those with noprogressTopic.openActionholdsdownloadingfor the length of the transfer, and there is no cancel — only the read timeout. Out of scope for the run lock, but this PR is what splitdownloadingout, so it is the last asymmetry left from that split.Standing
Four units now extracted and pinned —
ProbeCadence,RunClaim,LegacyEnding,navStillWanted— and this round produced no new defect in any of them. Every finding above is either about how the ViewModel calls them (S20, S22) or about the primitive's own guarantees (S21).runAction,submitForm,finishRunandendRunstill have no test, but they are visibly thinner than five rounds ago: the rules moved out, and the findings moved with them.Fix S21 — it is small — and say which way you want S20 to go, and I will approve.
Re-filing my review of
3498205as REQUEST_CHANGES so the state matches the intent: nothing here is blocking, but this is the last cleanup pass and I would rather it be a checklist than a suggestion. Reasoning for each item is in the review above (#92); this is the list.Final cleanup
S21 —
RunClaim: make the read-modify-write atomic.claim()andrelease()are get-then-set on anAtomicReference. Main-only makes it correct; the class doc ("[claim] is the mutual exclusion itself") plus the atomic container invites the opposite assumption.compareAndSetin a small loop, orupdateAndGet. This is the primitive the last four rounds converged on — it should be the one place a reader does not have to check the callers to know it is safe.S20 — decide when the successPage request is consumed.
pageOnScreenno longer takes part in the navigation decision at all; freshness-at-consumption is the whole rule, and consumption is aLaunchedEffect. That is exactly right for the back-stack case. For the app being backgrounded, composition is retained and whether the effect fires promptly is the recomposer's business, not this code's.repeatOnLifecycle(STARTED)around the consumption, or apageOnScreencheck insidepageOpened()— either is fine, but pick one so the behavior is stated rather than inherited.S22 — do not report a failure the app does not have.
onFailurecallsending.responded(), discards the answer, and writesactionError = e.message. A POST that times out after the stream already said "done" puts "timeout" over work that visibly completed and was already refetched. Whenresponded()returns true the ending is known; the transport error belongs in the feed, not the verdict banner.S23 — move
navStillWantedtodata/follow/. It is the only extracted rule still living inSchemaViewModel.kt, alongside three that were moved for exactly this reason.Nits, take or leave: a refused
claim()is a silentreturn— reachable via a confirm dialog opened before something else took the state, where Confirm now closes and does nothing with no explanation;LegacyEndingis allocated for actions with noprogressTopic; downloads remain the one wedge with no exit (openActionholdsdownloadingfor the whole transfer, only the read timeout ends it) — out of scope for the run lock, but this PR is what splitdownloadingout; and there is still no ktlint/detekt/spotless gate on ~2000 lines.Everything else stands as written in #92: B7 and B8 are properly closed — I walked all four
LegacyEndingorderings and traced the composition lifecycle behindpageOpened(), rather than taking the commit message for either — andreleaseRun(token, …),claim(): Int?andowns()requiringheldare the right shapes for S16/S17/S19.Post these and I will approve.
Checklist posted at
db9ee59../gradlew assembleDebug,testDebugUnitTest(105 tests) andlintDebuggreen.1. S21 —
RunClaimis atomic.claim()is acompareAndSetloop (the refusal and the take are one act only if nothing changed between them; a losing CAS re-reads and refuses on the next pass if a rival got there first),release()anupdateAndGet. The class doc no longer leans on Main-only as the reason it is safe — it says every read-modify-write is a CAS and why.RunClaimTestgained two racing cases: 16 threads claiming a free state (exactly one token, and it owns) and 16 concurrent releases (the epoch advances by 16, so no release is lost). Not a proof of thread-safety, and the test says so in as many words — it is the shape get-then-set loses.2. S20 — consumption is gated on STARTED.
repeatOnLifecycle(Lifecycle.State.STARTED)aroundpageOpened(). Chosen over apageOnScreencheck insidepageOpened()because that one spends the request while the operator is away: a two-second app switch at the wrong moment would drop the navigation for good, which is exactly whatSUCCESS_NAV_GRACE_MSexists to prevent. Gating the consumption instead makes both ways of being gone land on the same rule — a back-stack entry leaves composition and is consumed on return, a backgrounded app keeps its composition but not its STARTED state — so the duration the grace measures is a page that was on screen.pageOpened's doc now states that rather than leaving it to the caller.3. S22 — the transport error goes in the feed.
onFailurekeeps the answer fromresponded()now: when the stream already said "done" and the refresh already ran, the failure is pushed as a feed line (request failed: …) and the claim is released with no verdict. The banner is only written when the response is genuinely the ending.4. S23 —
navStillWantedmoved todata/follow/SuccessNav.kt, besideProbeCadence,RunClaimandLegacyEnding. Test import followed it.5. Nits
"another action is still running on this page"— page banner for an action, form banner for a submit. The exit from whatever holds the lock is already on screen, so the message only has to say the press was not lost.LegacyEndingis allocated only for a legacy action (action.progressTopic?.let { LegacyEnding() }), and its nullness now IS "there is a second ending to wait for" — the branch below readsending == nullinstead of re-deriving it fromprogressTopic, so the two facts cannot drift.cancelDownload()cancels the job. It releases the screen, not the socket — the bytes are read blocking, so the read runs to its own end: if it completes the file is saved anyway and only the screen moved on; if it fails, both download paths already delete their partial file. Two things fell out of that and are handled rather than left implicit: the abandoned transfer'sonProgresskeeps firing, so atransferSeqretires its samples at cancel time (otherwise it paints over the next download's line — the same failure the run token exists to stop); andopenActionnow also guards on the job, because the label it used to check is written inside the coroutine and two presses in the same frame both read it as free.runCatchingcatches the cancellation too, so it is rethrown and afinallyclears the transfer state — an operator who pressed the button is not told the download failed.Not done: the ktlint/detekt/spotless gate. Deliberate, and the one item I would rather argue than quietly skip: adding it means reformatting every file in the app, not the ~2000 lines this PR touches, and that diff would bury the change it is attached to. It is a repo-wide decision that deserves its own PR — I will open one against
mainafter this lands, so the reformat is reviewable as a reformat.Review at
db9ee59../gradlew testDebugUnitTestgreen.Every item on the #93 checklist is closed, and the two structural ones are closed properly — I checked the mechanisms, not the commit message:
claim()is a real CAS loop with the refusal inside it, andrelease()isupdateAndGet, so the epoch bump is atomic too. The two threaded tests are the right pair (one winner among racers; N releases issue N epochs) and the comment is honest that a race test proves a shape, not thread-safety.repeatOnLifecycle(STARTED)on the NavBackStackEntry's lifecycle is exactly the right gate: an entry under another page and a backgrounded app both drop below STARTED, so the two "operator is gone" cases land on one rule and the recomposer's scheduling stops being load-bearing.pageOpened()clearing unconditionally makes the repeat idempotent if the lifecycle bounces before the key changes.BUSYcloses the silent refusal.LegacyEndingallocated only when there is a stream, andending == nullreplacingaction.progressTopic == nullcollapses "is there a second ending" to one fact rather than two that can drift.Then the download exit — which came out of a throwaway nit — was implemented, and it does not work.
Blocking
B9 — "Stop download" releases nothing
cancelDownload():865cancelsdownloadJob, nulls the field, bumpstransferSeq. It never touches_state. Everything the operator can see is cleared in the coroutine'sfinallyat:842-848— and thatfinallycannot run until the coroutine actually unwinds.It doesn't unwind.
ApiClient.moduleDownload:195-205copies the body insidewithContext(Dispatchers.IO)through aForwardingSourcewith noensureActive()anywhere in the read loop:Job.cancel()on a coroutine blocked in a non-cancellable read does nothing until the read returns. So after the press:state.transferringstays true → the "Downloading …" line and the "Stop download" button stay on screen;state.downloadingstays non-null →openAction:795refuses every new transfer;The KDoc says "Releases the screen; it does not stop the transfer." It does neither. This is the wedge the button was added to open, still open, now with a control that claims otherwise.
The fix is the pattern already built twice in this PR, applied to the release path instead of only the write path:
cancelDownload()clearsdownloading/transferring/transferPercentitself — the same thingendRundoes for the run lock, and for the same reason: the exit must be the thing that frees the screen, not a consequence that arrives when the work ends anyway.finallyat:842must then be guarded onseq == transferSeq.get(). Once (1) lands, the operator can start download B while A is still blocked in its read, and A's unguardedfinallywill clear B's transfer state.reportTransfer:1007already carries exactly this check for the write path; the teardown needs the same one.Should fix
S24 —
start()reaches every wait except the transfer.:400-409cancelsrefreshJobs,runJob,progressJoband callsclaim.release(). It does not canceldownloadJoband does not bumptransferSeq. So afterretry(), an in-flight transfer'sreportTransferstill matches the current seq and writestransferPercentonto the freshly reset state, and itsfinallylater clears transfer state on a page that has none. The run lock got the full treatment instart(); the transfer got none of it.S25 — the
BUSYbanner never clears.releaseRun(error)preservesactionErrorwhenerroris null (actionError = if (error != null && !toForm) error else it.actionError), which is right for "a release must not wipe an unrelated failure". ButBUSYis not an unrelated failure — it is a statement about the lock that becomes false the moment the lock opens. Press Confirm on a dialog while something runs, wait for that something to finish, and the page still reads "another action is still running on this page". Clear it on release, or carry it as its own short-lived field rather than as an error.Standing
Five units extracted and pinned now —
ProbeCadence,RunClaim,LegacyEnding,navStillWanted, and the transfer's own seq counter — and none of them produced a finding this round. B9 is in the same place as every finding for the last six rounds: the glue inSchemaViewModelthat has no test. It is also the third time the same lesson has come back in a new costume — a writer that keeps writing after the thing it belongs to was abandoned — and this time the token existed and was simply not applied to the teardown.Fix B9 and I will approve; S24 and S25 are small enough to ride along or to follow.
Pushed
a2df357../gradlew assembleDebug,testDebugUnitTest(108) andlintDebuggreen.B9 — the exit frees the screen. Both halves, as prescribed.
cancelDownload()clearsdownloading/transferPercent/transferringitself. The KDoc now says why that has to be the exit's own job rather than the teardown's: cancelling a coroutine blocked in a non-cancellable read is a request the read never reads, so thefinallycould not run until the body ended — and the diagnosis (ForwardingSource, noensureActive(), a slow steady stream never tripping the read timeout) is written at the site so the next person to move this code knows the button is load-bearing.The
finallyis gated onseq == transferSeq.get(), the same checkreportTransfercarries. The success tail above it is gated too, on the same reasoning applied to the same coroutine: an abandoned transfer that completes would otherwise write its "Saved …" line and itsactionErroronto whatever claimed the screen next — and open a viewer for a file the operator stopped waiting for. The file still lands either way; only the screen moved on, which is what the KDoc already promised.Not done, deliberately:
ensureActive()inApiClient.moduleDownload's read loop. It would make the cancel real rather than a release, but it also changes what a cancelled download is — today it completes and the file is saved, and both paths delete a partial file on failure, so a cancellable read turns "you stopped watching" into "you threw the bytes away". That is a product decision about the button, not the wedge this finding is about, and it belongs with the transfer's own PR.S24 —
start()callscancelDownload(), next to the run-lock teardown it already had. The whole reset is one list again.S25 —
releasedBanner(error, current), top-level and pure: the new verdict if the release carries one, otherwise what was already there — exceptBUSY, which is a statement about the lock the release just opened.ReleasedBannerTestpins all three, including the case you described (Confirm on a dialog while something runs, wait it out, page still claims a run is in flight).Standing. No new extraction this round — the three findings landed on state that already had its token (
transferSeq) and simply was not presenting it at the teardown, which is your read of it exactly. The ViewModel fixture remains the open item, unchanged.Review at
a2df357../gradlew testDebugUnitTestgreen.B9, S24 and S25 are all closed, and B9 more thoroughly than I asked: the seq guard went on the success path as well as the teardown, so a cancelled download can no longer pop an
ACTION_VIEWIntent minutes after the operator walked away.releasedBanneras a pinned function withBUSYas its one named exception is the right shape — the rule is now stated once instead of being an inline condition nobody could point at.The exit works now. What it opened is a second writer, and one of the two download paths is not ready for one.
Blocking
B10 — cancel-then-retry corrupts the saved file on API 26–28
downloadToAppDownloads:1023streams into a temp path derived only from the URL:Before this commit that was safe by accident:
downloadingstayed non-null after a cancel, so no second transfer could start.cancelDownload():913now frees the guard the moment it is pressed, while the abandoned coroutine is still blocked insidemoduleDownload's read loop. Press the same action again — the obvious thing to do after cancelling a stalled download — and two transfers open.<name>.partandwriteAllinto it concurrently.Whichever finishes first renames it to a
uniqueNamefinal name; the loser's fd follows the inode, so it keeps appending into the file that was just published. The app then reports a saved file whose bytes are not what the server sent, with nothing to indicate it.uniqueNameguards the final name and nothing guards the temp one.Q+ is fine — each transfer does its own
resolver.insertand gets a distinct URI, and MediaStore dedupes — so this is the pre-Q path only.minSdk = 26, so that is a live range, and the seq is already threaded into both download functions:".$fallbackName.$seq.part"closes it.Should fix
S26 — freeing the screen leaves the work unbounded, and it did not have to.
cancelDownloadcancels a coroutine that cannot observe cancellation:ApiClient.moduleDownloadreads through a plainForwardingSource, andwithContext(Dispatchers.IO)only throws once the blocking read returns on its own. It also setsdownloadJob = null, soopenAction:816re-opens immediately. Every cancel-and-retry therefore stacks another blocked read holding an IO thread and an OkHttp connection until the read timeout expires.The KDoc presents "it does not stop the transfer" as a deliberate trade, but it is not a forced one. Keeping the
Calland cancelling it —okHttp.newCall(req)held,currentCoroutineContext().job.invokeOnCompletion { call.cancel() }around thewithContext, or anisActivecheck inside theForwardingSource.readoverride — actually aborts the socket. That bounds the abandoned work, and it also removes most of B10, because there is no longer a second writer to collide with.Nits
releasedBannerandBUSYare pure,internal, and now pinned by a test — and they live inSchemaViewModel.kt, whilenavStillWantedwas moved todata/follow/last round for precisely that reason. One rule or the other.ReleasedBannerTestpins the function. The part that historically broke is the routing around it —releasedBanner(if (toForm) null else error, …)across two banners — which is still an untested expression in the ViewModel.start()callscancelDownload()and then_state.value = SchemaUiState(), so the state write insidecancelDownloadis immediately superseded. Harmless, just dead.Standing
Six units extracted and pinned, none of which produced a finding this round. B10 is in
SchemaViewModel's glue like every finding before it — but it is a different kind: the first one in several rounds that is not about ownership of shared state. The exit for the transfer is now correct; what is missing is that making a wait abandonable turns "one at a time" into a real concurrency question, and the pre-Q path answers it with a fixed filename.Fix B10 and I will approve. S26 is the better version of the same fix if you want to do it once.
Review at
08eddbf../gradlew testDebugUnitTestgreen.S26 is done properly on the download side —
invokeOnCompletion { call.cancel() }closes the socket the blocking read is actually parked on, which is the only thing that could have made cancellation real, and thedispose()infinallykeeps a completed transfer from cancelling its own finished call.releasedBanners+Bannersmoved todata/follow/with the routing now pinned by tests (including the both-sidesBUSYcase, which is the one I would have forgotten).abandonDownload/cancelDownloadsplits the dead write out ofstart().B10 is narrowed, not closed. The uniqueness you added is per-ViewModel, and the directory is per-device.
Blocking
B11 — the per-transfer temp name is unique within one page, and two pages share the filesystem
downloadToAppDownloads:1028is now:seqcomes fromtransferSeq(:331), which isprivate val transferSeq = AtomicInteger(0)— an instance field ofSchemaViewModel, and there is one ViewModel per module/subPath, several of them alive on the back stack at once. Each starts counting at 0.So: page A downloads
backup.tar.gz→.backup.tar.gz.1.part. Operator navigates to page B — A's ViewModel and its download both survive on the back stack,start()is not called again, and the in-flight guard is per-instance — and downloads a file with the same basename →.backup.tar.gz.1.part. Two writers, one inode, and the same ending as B10: one publishes under auniqueNamefinal name and the loser's fd keeps appending into the file the operator was just handed.fallbackNameisfileNameOf(path), a basename off a module-relative route, so two modules servingconfig.yamlorbackup.tar.gzis not a stretch in a module-agnostic UI. And because both counters walk 1, 2, 3… in step, cancel-and-retry on both pages makes a collision more likely, not less.The fix wants a name unique to the device, not to the page:
File.createTempFile(".$fallbackName.", ".part", dir)gets it from the OS, andseqgoes back to being purely an ownership token. Q+ is unaffected — every transfer does its ownresolver.insertand gets its own URI.Should fix
S27 — the cancellable-request fix landed on the download and not on the upload.
moduleDownload:179holds itsCalland cancels it;modulePostMultipart:130is still a bareokHttp.newCall(req).execute().use { … }. So "Stop waiting" on a form submit does exactly what "Stop download" did before this commit: frees the screen and leaves a blocking write holding an IO thread and a connection until a timeout.abandonRun's KDoc still says so out loud — "An in-flight request is not cancelled either" — and the upload is the longer of the two waits, which is the entire reason B5 existed.It is the same five lines. The other
execute()sites (modulePost:100,moduleGet:87,taskRun:239,cancelRun:257) are short enough not to care, so this is one method, not a sweep.This is the fourth time in this PR a fix has been applied to one of two symmetric paths — S4/S8 (refresh topic vs log follow), B1/B4 (action vs submit), and now this. Worth doing both at once.
Nits
.partfiles now accumulate. A fixed temp name meant at most one stale partial per filename; a per-transfer name means a process death leaves.foo.3.partbehind for good, and nothing ever sweeps the directory.createTempFilemakes this worse, not better, so it is worth adir.listFiles { … endsWith(".part") }cleanup onstart()— or at least an acknowledgement that this directory grows.invokeOnCompletioncomment says the dispose exists "so a completed download does not leave a handler on a job that outlives it". The job is thewithContext(Dispatchers.IO)block's own child and completes immediately after, so nothing outlives anything — the dispose is load-bearing for a different reason, namely that without it a successful transfer firescall.cancel()on its own completion. Right code, wrong reason on the label.BUSYis user-facing English living indata/follow/. Consistent with the rest of the codebase (no string resources anywhere), so this is a note rather than a request — but adatapackage is where it will be least expected when localisation eventually happens.Standing
Seven units extracted and pinned; none produced a finding this round, and
ReleasedBannerTestin particular now covers the half that historically broke. B11 is the same defect as B10 seen from one level up — the fix scoped uniqueness to the object that owns the counter rather than to the resource being contended, which is the filesystem. That is the recurring shape of this whole review: the guard is correct for the writer it was written next to, and the thing being guarded is shared wider than that writer.Fix B11 and I will approve. S27 is small and belongs in the same commit.
Pushed
bad6e49../gradlew assembleDebug,testDebugUnitTest(112) andlintDebuggreen.B11 — the name now comes from the thing that owns the directory.
File.createTempFile(".$fallbackName.", ".part", dir). You are right that scoping uniqueness to the counter was scoping it to the wrong object:transferSeqis an instance field, one ViewModel per module/subPath, several alive on the back stack, all counting from 0 in step — it can name what this page owns and nothing the filesystem sees. Its KDoc says that now, so the next person reaching for it as a name finds the reason it is not one.It is a behaviour change in one respect worth naming: unlike the
File()it replaces,createTempFilecan throw. An unwritable directory now fails one step earlier — before the first byte instead of at it — into the same banner, and the three-character prefix it demands is whyfileNameOfnever returning empty is a requirement rather than a nicety. Both written at the site.The orphan nit, taken — and it is the price of B11, not a separate thing. A fixed name left at most one stale partial per filename; a per-transfer name leaves a new one on every process death, and
createTempFilemakes that worse rather than better exactly as you said.start()sweeps the directory, on age rather than ownership: another page's in-flight download owns a file this sweep can see, and deleting an inode a writer still holds costs that writer the rename it ends with — but a live transfer writes, and writing moves the modification time, so an untouched-for-a-day partial has no writer left.isOrphanPart(name, lastModifiedMs, nowMs)is the rule, pure and pinned inDownloadNamingTestbesideuniqueName, because it is a predicate that deletes files.lastModified() == 0is the filesystem saying it does not know and is not read as "long ago"; neither is a clock that moved backwards.S27 — both halves, since the ApiClient five lines alone would not have cancelled anything.
modulePostMultipartholds itsCalland cancels it on completion, same asmoduleDownload. But the submit coroutine was untracked:viewModelScope.launchwith nobody holding theJob, so there was nothing for the exit to cancel. It issubmitJobnow, cancelled byendRun(every exit, "Stop waiting" included) and bystart()— a page turn abandons an upload for the same reason the button does. Safe to cancel because every continuation in there was already gated on the claim token: a cancelled submit finds its token retired and reports nothing, which is what abandoning it means.followRunlaunches onviewModelScope, not as a child of the submit, so a submit that handed over to a run does not take the follow down when its job is cancelled.abandonRun's KDoc no longer says the request survives. The multipart progress callback keeps its token check, with the reason corrected: samples can still arrive between the release and the socket closing, so the check is what stops them painting over the next claimant — the same thing it did before, for a smaller window.Nits.
invokeOnCompletionlabel: your read is right, and the comment now says it. Nothing outlives anything — the handler is on the block's own job. The dispose is load-bearing because that job completing normally would otherwise firecall.cancel()on a transfer that just succeeded.BUSYindata/follow/: left where it is. Agreed it is the wrong neighbourhood for a user-facing string, and the answer is string resources rather than moving one constant to a better-looking package — that is a repo-wide change, and it would be the second one I am deferring rather than doing.Standing. The
versionNamestays at 0.31.0 (unreleased branch), and the ktlint/detekt gate is still deliberately out, for its own PR after this lands. The ViewModel fixture remains the open item; this round's new logic went intoisOrphanPartrather than into the coroutine, which is the eighth unit extracted for that reason.🤖 Generated with Claude Code
Review at
bad6e49../gradlew testDebugUnitTestgreen.B11 and S27 are closed on the paths they name.
File.createTempFileis the right answer for the right stated reason — the contended thing is the directory, not the object holding the counter — and I checked the prefix concern the comment raises:fileNameOf:253ends in?: "download"andsanitiseFilenamenever returns empty, so the shortest possible prefix is.X.and the three-character floor holds.isOrphanPartguards bothlastModified == 0and a clock that moved backwards, and both are tested. TheinvokeOnCompletioncomment now gives the real reason for itsdispose().The sweep those temp names made necessary deletes saved downloads.
Blocking
B12 —
sweepOrphanPartsdeletes the operator's filesisOrphanPart:307decides on the name alone:and
sweepOrphanParts:476runs it overgetExternalFilesDir(DIRECTORY_DOWNLOADS)— the same directory finished downloads are saved into.downloadToAppDownloads:1135renames the temp toFile(dir, finalName)in that exact directory, andfinalNamecomes from the server:content-disposition, or the URL basename, throughsanitiseFilenameanduniqueName, neither of which touches the extension.So on API 26–28: download any file whose name ends in
.part, and 24 hours later the app silently deletes it. This repo shipsmodule_filebrowser— a module whose whole job is letting the operator download arbitrary files off the server — so "a file namedsomething.part" is not a hypothetical, it is a category.The temp files all begin with
.(thecreateTempFileprefix at:1120is".$fallbackName."), soname.startsWith(".") && name.endsWith(".part")is the one-line version. The robust version is a subdirectory — write partials intodir/.parts/and sweep only that — which makes it structurally impossible for the sweep to see a saved file at all, including the.foo.partcase the dot check still misses.DownloadNamingTestreinforces the gap rather than catching it: its "not a partial" case isbackup.tar.gz, which was never at risk. The case that matters — a saved file ending in.part— is not in the test.Should fix
S28 — the cancellable-request fix is now half-applied for the third time.
modulePostMultipart:130andmoduleDownload:179both hold theirCalland cancel it.modulePost:100is still a bareokHttp.newCall(req).execute().use { … }, andrunActionhas no job field at all — onlysubmitJob:363exists, cancelled inendRunandstart().So "Stop waiting" on an action still frees the screen and leaves a blocking request holding an IO thread and a connection until a timeout. And that is not the short case:
LegacyEndingexists precisely because a module can do its work inside its own request and answer afterwards, which is the long POST that most needs the exit to reach it.abandonRun's KDoc was narrowed from "An in-flight request is not cancelled either" to "An in-flight submit IS cancelled, request included" — accurate about the submit, and it quietly stops mentioning the action. Track the action's job the same way and the sentence can go back to saying "request".Nits
submitJobis never nulled on normal completion, so a finishedJobstays referenced until the next submit. Trivial, butrunJob/progressJob/downloadJoball null themselves and this one does not.start()— every page-ViewModel creation — listing a directory to clean up after a failure mode that only happens on process death. Once per process would do.PART_ORPHAN_MSandisOrphanPartare new pure, pinned rules that stayed inSchemaViewModel.kt, whilenavStillWantedandreleasedBannerwere moved todata/follow/for being exactly that. Defensible —uniqueNameandfileNameOfalready live here, so download naming has its own home — but the file is now where both the extracted and the un-extracted rules live, which is the state the moves were meant to end.Standing
B12 is the same shape as B10 and B11 one more time, and it is worth naming plainly because it has now happened three commits running: a mechanism was made safe for the writer in front of it (unique temp names per transfer), the fix created a new obligation (partials that never get overwritten), the obligation was met with a rule scoped to that mechanism (delete
*.part), and the rule reaches further than the mechanism does (into the directory where finished downloads live).Each individual step was correct. The check that keeps being skipped is the last one — what else is in the blast radius of the thing I just added.
Fix B12 and I will approve; S28 belongs in the same commit.
b1c13f5answers #101../gradlew testDebugUnitTestandassembleDebuggreen.B12 — the sweep can no longer see a saved file
Fixed the structural way, not the one-line way. Partials now live in
downloads/.parts/(PARTS_DIR):createTempFiletargets that subdirectory, and the rename at the end of a transfer is the only way a file leaves it. The traversal moved into a puresweepParts(partsDir, nowMs)that reads that directory and nothing else — so whateverisOrphanPartsays about a name, a saved file one directory up is unreachable,.foo.partincluded. The name check stays insideisOrphanPartas belt over the braces.The test now exercises the case that was missing, with real directories rather than the pure rule alone: a saved file literally named
something.part, older thanPART_ORPHAN_MS, sitting in the downloads root, survives a sweep that deletes a day-old orphan in.parts/and keeps a live partial being written now.S28 — the third half-application, completed
modulePostholds itsCalland cancels it frominvokeOnCompletion, disposed infinally— the same mechanism as its multipart and download twins, for the same reason.runActiontracks its job (actionJob), andendRun/start()cancel it alongsidesubmitJob. So "Stop waiting" on an action ends the request itself, which matters most for exactly the case you named: aLegacyEndingmodule doing its work inside its own POST.abandonRun's KDoc says "request" again, now covering both write paths.Nits
submitJobandactionJobnull themselves on completion (invokeOnCompletionwith an===guard, so a job that was already replaced does not null its successor).partsSwept, file-levelAtomicBoolean) — orphans are made by process death, so that is how often new ones can appear. One wrinkle found in review before pushing: the flag was taken before the work ran, so a launch cancelled before the dispatcher got to it (a quick back press destroying the ViewModel) would have blocked sweeping for the process lifetime. The completion handler hands the flag back on cancellation.uniqueNameandfileNameOf.On the standing note: taken. The check that kept being skipped — what else is in the blast radius of the thing just added — is why B12 is closed by scoping what the sweep can see rather than sharpening what it looks for.
Review at
b1c13f5../gradlew testDebugUnitTestgreen. Approving.B12 and S28 are closed, and B12 with the structural fix rather than the one-liner. I checked the parts that could still have gone wrong:
PARTS_DIRputs partials one directory down andsweepPartsis scoped to it, so a saved file is not "one naming convention away" from the sweep — it is unreachable by it.sweepPartsextracted as a pure traversal over aFileis the right seam, and the test is the one I would have written: a savedsomething.partsurvives a sweep that deletes a real orphan and leaves a live partial alone.uniqueName(name) { File(dir, it).exists() }still probes the downloads directory, not the parts one — a detail that would have been easy to move by accident. It also, incidentally, keeps a file from being named.parts.partsSweptas a process-wideAtomicBooleanmatches the actual invariant (only process death makes an orphan), and handing the flag back on cancellation is the part that would otherwise have meant a process that never sweeps.modulePostnow cancels itsCall,actionJobis tracked and cancelled in bothendRunandstart(), and both job fields self-null with an===guard so a later claimant's job is not cleared by an earlier one's completion.abandonRun's KDoc says "request" again and now means it.Cancellation of an in-flight POST is safe for the reason the KDoc gives — I traced it rather than assuming: the cancelled coroutine's
foldstill runs (itsrunCatchingswallows theCancellationException), and every branch is behindclaim.owns(token), whichreleaseRunhas already retired by then because cancel is asynchronous and release is not.Nothing blocking left. What follows is for later, not for this PR.
Follow-up
S29 — the final name has the same race the temp name just lost.
Two transfers can now genuinely run at once — two pages on the back stack, each with its own ViewModel and its own guard — and if they resolve the same
finalNameneitherexists()at check time, so both compute the same candidate and the secondrenameTosilently replaces the first. That is exactly whatuniqueName's own comment says it exists to prevent, one level down from where B11 was fixed: the check and the claim are two steps, and the filesystem is the shared thing between them.This is pre-existing rather than something this PR introduced — a per-ViewModel guard never bound cross-page downloads.
minSdkis 26, soFiles.move(temp.toPath(), target.toPath())withoutREPLACE_EXISTINGthrowsFileAlreadyExistsExceptionatomically, and retrying with the next candidate makes the loop a claim instead of a poll.Nits
runActionandsubmitFormwrap their POST inrunCatchingwithout rethrowingCancellationException, whileprobe()andopenActionboth do. Harmless here — the token guard catches it — but the rule this PR established has two exceptions left in it.downloadJobis now the only job field without the self-nullinginvokeOnCompletionthe other two just got. Functionally fine (isActiveis false on a completed job), but it is the odd one out.IS_PENDINGrow that this code never deletes and the platform expires after seven days. The asymmetry is fine — the doc just claims slightly more than the code does.On the whole
Thirteen rounds. The shape that produced almost every finding was one thing: a guard correct for the writer in front of it, over state shared wider than that writer —
runningActionacross two entry points, the epoch across four response branches, the seq across two ViewModels, the sweep across one directory. What ended it was not more guards; it wasRunClaim,ProbeCadence,LegacyEnding,releasedBanners,navStillWanted,isOrphanPart/sweepParts— seven rules pulled out to where they could be named and pinned. Every one of them stopped generating findings the round it was extracted, and the last three rounds' findings were all in the shrinking remainder.SchemaViewModelstill has no test of its own and is still where anything left would hide. But the rules that used to live in it don't any more, and the ones that moved out are the ones that were wrong.Ship it.