feat: follow host-owned task runs from schema actions #4

Merged
julian merged 14 commits from feat/task-runs into main 2026-08-13 23:10:40 +02:00
Owner

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, runAction cleared runningAction and 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 runId means the work is not over, so the app follows core:task:<runId> and lets the run's terminal status end the action. Both write paths do this — runAction and submitForm.

That topic is retained, which makes this better than the stream it replaces rather than merely equivalent:

progressTopic task run
subscribe after POST race — frames before the subscription are lost snapshot is the whole run so far
already-finished run nothing to show renders identically
end of run a log line starting done/error terminal status
app backgrounded pushes lost resumes from since

LiveClient learns retained topics to make that possible: the snap frame, the per-topic tseq it carries, and the epoch scoping that sequence 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 up:

  • phase is now carried forward on merge and read: only the download half is summed, because compressed and uncompressed byte counts must never be added together.
  • ProgressKind gained the kinds core names (pull, step) plus an OTHER default. The wire format allows any string there, and an unknown kind used to take the whole event down with it.

progressTopic stays. 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:

  • No WebSocket at all (proxy strips 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 over GET /api/core/tasks/:id every 3s instead.
  • A hole in the subscription: an event the collector never received sets gap, which withholds since from the next sub frame (a subscription with a hole must not promise it has everything through tseq), 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 in SubState, testable without a socket.
  • A status this build cannot name: finishedAtMs ends the follow whatever RunStatus.UNKNOWN implies.
  • A run that outlives the operator's patience: capabilities.cancel is 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: path actions share the feed and usually the server-side lock, navigation entries and open downloads share neither. Downloads now have their own busy state.

Deliberately not handled

  • Withheld fields. A viewer sees empty entities/lines with their names in withheld. The status still lands, so the button still finishes; the rows are simply absent.
  • step and checkpoint events. Of the five event types on the topic, status, progress and log are rendered; step and checkpoint are dropped. RunStep is 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.
  • A form's successPage when 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.
  • Two actions in flight. progress and runningAction are single-valued; the model rejects a second action while one runs rather than letting the two share a feed.

followRun is deliberately not passed through RefreshPolicy.gate — its own doc says not to gate user-initiated flows like action progress, and the existing progressTopic path does not either.

Verification

./gradlew assembleDebug and ./gradlew testDebugUnitTest green (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/epoch on the frame), ProgressFeed.of (order, last-wins, the 500-line tail) and what ends a run (null snapshot, unknown status, finishedAtMs, failure wording). followRun's coroutine wiring itself is not unit-tested — the ViewModel needs an Application and the container, and this module has neither Robolectric nor coroutines-test.

Not yet exercised against a live server — the modules that answer { runId } from a schema action are the companion PR (module_updates), and module_systemd / module_ipmi deliberately still answer a synchronous verdict on main until this lands.

🤖 Generated with Claude Code

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`, `runAction` cleared `runningAction` and 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 `runId` means the work is not over, so the app follows `core:task:<runId>` and lets the run's terminal status end the action. Both write paths do this — `runAction` and `submitForm`. That topic is *retained*, which makes this better than the stream it replaces rather than merely equivalent: | | `progressTopic` | task run | |---|---|---| | subscribe after POST | race — frames before the subscription are lost | snapshot is the whole run so far | | already-finished run | nothing to show | renders identically | | end of run | a log line starting `done`/`error` | terminal status | | app backgrounded | pushes lost | resumes from `since` | `LiveClient` learns retained topics to make that possible: the `snap` frame, the per-topic `tseq` it carries, and the `epoch` scoping that sequence 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 up: - `phase` is now carried forward on merge and read: only the `download` half is summed, because compressed and uncompressed byte counts must never be added together. - `ProgressKind` gained the kinds core names (`pull`, `step`) plus an `OTHER` default. The wire format allows **any** string there, and an unknown kind used to take the whole event down with it. `progressTopic` stays. 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: - **No WebSocket at all** (proxy strips `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 over `GET /api/core/tasks/:id` every 3s instead. - **A hole in the subscription**: an event the collector never received sets `gap`, which withholds `since` from the next `sub` frame (a subscription with a hole must not promise it has everything through `tseq`), 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 in `SubState`, testable without a socket. - **A status this build cannot name**: `finishedAtMs` ends the follow whatever `RunStatus.UNKNOWN` implies. - **A run that outlives the operator's patience**: `capabilities.cancel` is 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: `path` actions share the feed and usually the server-side lock, navigation entries and `open` downloads share neither. Downloads now have their own busy state. ## Deliberately not handled - **Withheld fields.** A viewer sees empty `entities`/`lines` with their names in `withheld`. The status still lands, so the button still finishes; the rows are simply absent. - **`step` and `checkpoint` events.** Of the five event types on the topic, `status`, `progress` and `log` are rendered; `step` and `checkpoint` are dropped. `RunStep` is 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. - **A form's `successPage` when 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. - **Two actions in flight.** `progress` and `runningAction` are single-valued; the model rejects a second action while one runs rather than letting the two share a feed. `followRun` is deliberately **not** passed through `RefreshPolicy.gate` — its own doc says not to gate user-initiated flows like action progress, and the existing `progressTopic` path does not either. ## Verification `./gradlew assembleDebug` and `./gradlew testDebugUnitTest` green (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`/`epoch` on the frame), `ProgressFeed.of` (order, last-wins, the 500-line tail) and what ends a run (`null` snapshot, unknown status, `finishedAtMs`, failure wording). `followRun`'s coroutine wiring itself is not unit-tested — the ViewModel needs an `Application` and the container, and this module has neither Robolectric nor `coroutines-test`. Not yet exercised against a live server — the modules that answer `{ runId }` from a schema action are the companion PR (`module_updates`), and `module_systemd` / `module_ipmi` deliberately still answer a synchronous verdict on main until this lands. 🤖 Generated with [Claude Code](https://claude.com/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>
thisilike requested changes 2026-08-12 13:17:44 +02:00
Dismissed
thisilike left a comment

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 in packages/modules/docker/frontend/, rather than against the description.

The retained-topic protocol work is right: the snap / tseq / epoch / since handling matches live.ts frame-for-frame, the Resync suppression for retained topics is correct, and the is LiveEvent.Snapshot -> Unit branch in startLogFollows is a valid assertion — providers are never retained. What follows is what does not hold.

Blockers

1. PULL is not a layer — the aggregate bar is now wrong

UpdateProgress.kt:169

val layers: List<ProgressEntity> get() = ofKind(ProgressKind.LAYER) + ofKind(ProgressKind.PULL)

kind: "pull" is docker's synthetic rollup, not a layer — compose_progress.ts:644 emits it with key: "pull/summary", id: "pull", and its numbers live in a pull field this model does not carry. It has no total and no fraction. 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 in ls.sumOf { it.fraction ?: 0.0 } / ls.size, and ls.size is now N+1. The bar is systematically understated — a single layer at 100% renders as 50%.
  • quiet = layers.size - shown.size in UpdateProgressCard counts 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. Drop PULL from layers; if the rollup is worth rendering it needs its own bucket that reads the pull object.

2. A null snapshot leaves the button spinning forever

SchemaViewModel.kt:833-846

core:task:*'s snapshot returns null when the run is not found (routes.ts:230-233: return run ? projectRun(run, viewer) : null). decodeFromString<TaskRun>("null") throws, getOrNull() swallows it, return@collect — and runningAction is never cleared. The docker reference handles exactly this case:

onSnapshot: (data) => {
  const raw = data as TaskRun | null;
  if (!raw) { this.running = false; return }   // pruned, or never existed

Same class of hole: a rejected subscription. LiveClient.kt:234 removes the sub from subs and logs a warning — the flow then goes silent forever and is not re-subscribed on reconnect. Both paths need to reach finishRun (or an equivalent failure state) instead of going quiet.

3. followRun's job is never cancelled

SchemaViewModel.kt:870 puts it in refreshJobs, which is only cleared by start() on a page change. So the subscription survives the run:

  • one leaked WS subscription per action invocation, for the life of the page;
  • on reconnect the retained topic re-snapshots the (terminal) run, finishRun fires a second time, and refetchAll() runs again.

Cancel the job from finishRun, or hold it in a dedicated field the way progressJob is held.

Should fix

4. phase is carried forward but never read

UpdateProgress.kt:185-190

private val sizedLayers get() = layers.filter { it.total != null }
val transferred: Double get() = sizedLayers.sumOf { it.current ?: 0.0 }
val transferTotal: Double get() = sizedLayers.sumOf { it.total ?: 0.0 }

Download 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):

sized().filter((p) => p.phase === undefined || p.phase === "download");

Mobile still adds both. Carrying phase through the merge is a precondition for the fix, not the fix; the PR description claims the latter.

5. body?.jsonObject moved onto the success path

SchemaViewModel.kt:777

modulePost returns any JsonElement (ApiClient.kt:113 parses whatever came back). .jsonObject throws IllegalArgumentException on a non-object body, and this line sits outside the runCatching — uncaught in viewModelScope, so it takes the app down. Previously it only ran in the code >= 400 branch, 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.progress and runningAction are 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 with progressTopic the window was seconds; a dist-upgrade makes it thirty minutes.

7. A dropped frame is now unrecoverable

LiveClient.kt:225-231 advances sub.tseq before emit, and topic() is a callbackFlow — 64-slot buffer, trySend. An event dropped by buffer overflow still consumes its tseq, so the reconnect since will not ask for it back, and this PR removes the Resync that was the previous fallback. module_updates now emits one frame per apt line, which is the rate that overflows. Advance tseq only on a successful send.

Notes

  • finishRun treats skipped as success. That is right for onSelfConflict: drop, but skipped also arrives from a gate that can never open (whenMissing: "skip"), where it means the work silently did not happen.
  • feed.lines has no cap, and UpdateProgressCard renders one Text per line inside a single non-lazy Column. ProgressFeed.of alone seeds it with up to MAX_LINES = 500 from the snapshot, before any deltas.
  • RefreshPolicy.gate's doc does say not to gate user-initiated flows, so leaving followRun ungated is correct and matches the progressTopic path.
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 in `packages/modules/docker/frontend/`, rather than against the description. The retained-topic protocol work is right: the `snap` / `tseq` / `epoch` / `since` handling matches `live.ts` frame-for-frame, the `Resync` suppression for retained topics is correct, and the `is LiveEvent.Snapshot -> Unit` branch in `startLogFollows` is a valid assertion — providers are never retained. What follows is what does not hold. ## Blockers ### 1. `PULL` is not a layer — the aggregate bar is now wrong `UpdateProgress.kt:169` ```kotlin val layers: List<ProgressEntity> get() = ofKind(ProgressKind.LAYER) + ofKind(ProgressKind.PULL) ``` `kind: "pull"` is docker's *synthetic rollup*, not a layer — `compose_progress.ts:644` emits it with `key: "pull/summary"`, `id: "pull"`, and its numbers live in a `pull` field this model does not carry. It has no `total` and no `fraction`. 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 in `ls.sumOf { it.fraction ?: 0.0 } / ls.size`, and `ls.size` is now N+1. The bar is systematically understated — a single layer at 100% renders as 50%. - `quiet = layers.size - shown.size` in `UpdateProgressCard` counts 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. Drop `PULL` from `layers`; if the rollup is worth rendering it needs its own bucket that reads the `pull` object. ### 2. A null snapshot leaves the button spinning forever `SchemaViewModel.kt:833-846` `core:task:*`'s snapshot returns `null` when the run is not found (`routes.ts:230-233`: `return run ? projectRun(run, viewer) : null`). `decodeFromString<TaskRun>("null")` throws, `getOrNull()` swallows it, `return@collect` — and `runningAction` is never cleared. The docker reference handles exactly this case: ```ts onSnapshot: (data) => { const raw = data as TaskRun | null; if (!raw) { this.running = false; return } // pruned, or never existed ``` Same class of hole: a rejected subscription. `LiveClient.kt:234` removes the sub from `subs` and logs a warning — the flow then goes silent forever and is not re-subscribed on reconnect. Both paths need to reach `finishRun` (or an equivalent failure state) instead of going quiet. ### 3. `followRun`'s job is never cancelled `SchemaViewModel.kt:870` puts it in `refreshJobs`, which is only cleared by `start()` on a page change. So the subscription survives the run: - one leaked WS subscription per action invocation, for the life of the page; - on reconnect the retained topic re-snapshots the (terminal) run, `finishRun` fires a second time, and `refetchAll()` runs again. Cancel the job from `finishRun`, or hold it in a dedicated field the way `progressJob` is held. ## Should fix ### 4. `phase` is carried forward but never read `UpdateProgress.kt:185-190` ```kotlin private val sizedLayers get() = layers.filter { it.total != null } val transferred: Double get() = sizedLayers.sumOf { it.current ?: 0.0 } val transferTotal: Double get() = sizedLayers.sumOf { it.total ?: 0.0 } ``` Download 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`): ```ts sized().filter((p) => p.phase === undefined || p.phase === "download"); ``` Mobile still adds both. Carrying `phase` through the merge is a precondition for the fix, not the fix; the PR description claims the latter. ### 5. `body?.jsonObject` moved onto the success path `SchemaViewModel.kt:777` `modulePost` returns any `JsonElement` (`ApiClient.kt:113` parses whatever came back). `.jsonObject` throws `IllegalArgumentException` on a non-object body, and this line sits outside the `runCatching` — uncaught in `viewModelScope`, so it takes the app down. Previously it only ran in the `code >= 400` branch, 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.progress` and `runningAction` are 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 with `progressTopic` the window was seconds; a `dist-upgrade` makes it thirty minutes. ### 7. A dropped frame is now unrecoverable `LiveClient.kt:225-231` advances `sub.tseq` before `emit`, and `topic()` is a `callbackFlow` — 64-slot buffer, `trySend`. An event dropped by buffer overflow still consumes its `tseq`, so the reconnect `since` will not ask for it back, and this PR removes the `Resync` that was the previous fallback. `module_updates` now emits one frame per apt line, which is the rate that overflows. Advance `tseq` only on a successful send. ## Notes - `finishRun` treats `skipped` as success. That is right for `onSelfConflict: drop`, but `skipped` also arrives from a gate that can never open (`whenMissing: "skip"`), where it means the work silently did not happen. - `feed.lines` has no cap, and `UpdateProgressCard` renders one `Text` per line inside a single non-lazy `Column`. `ProgressFeed.of` alone seeds it with up to `MAX_LINES = 500` from the snapshot, before any deltas. - `RefreshPolicy.gate`'s doc does say not to gate user-initiated flows, so leaving `followRun` ungated is correct and matches the `progressTopic` path.
Three of these leave the button spinning or the numbers wrong; the rest are
holes the retained protocol opened and the review closed.

`kind: "pull"` is not a layer. It is docker's synthetic rollup over the layer
set — one entity, key "pull/summary", no total and no fraction, its numbers in
a `pull` object. Counting it as a layer put an extra zero-fraction slot in the
aggregate mean, so a single layer at 100% rendered as 50%, and it added one to
"N layers already present or queued". It decoded as a layer only because this
branch taught ProgressKind the name; before, the event was dropped. It is now
in no bucket at all, which is where the web feed puts it.

That rollup is also the answer to the byte counts. `phase` was carried forward
on merge but nothing read it, so download and extract bytes were still summed
— compressed against uncompressed, a figure that can exceed 100% of itself.
Filtering to the download phase alone is not enough either: a layer stops
reporting its download figure the moment it starts extracting, so the sum
would shrink as the pull progresses and vanish at the end. So PullSummary is
parsed and `transferred`/`transferTotal` come from it, with the download-phase
layer sum as the fallback for a server too old to send either it or `phase`.

A snapshot of a run core does not have is `null` (tasks/routes.ts: `run ?
projectRun(...) : null`) — pruned, or an id that never existed.
decodeFromString<TaskRun> threw, getOrNull swallowed it, and runningAction was
never cleared: the button spun for the life of the page over a run that was
never coming back. Same shape for a subscription the server refuses — the sub
was removed and the flow went silent forever — so LiveClient now emits
LiveEvent.Rejected before dropping it, and both paths end the action.

followRun's job was in refreshJobs, which only start() clears. One leaked
subscription per press, and on reconnect the retained topic re-snapshotted the
finished run and fired finishRun again. It has its own field now, cancelled by
the single terminal path every ending goes through.

`body?.jsonObject` moved onto the success path, where a route may answer any
JSON: .jsonObject throws on a non-object, outside runCatching, uncaught in
viewModelScope. `as? JsonObject` costs nothing.

tseq advanced before the emit, and topic() is a callbackFlow with a 64-slot
buffer and trySend. An event dropped by overflow still consumed its tseq, so
the reconnect's `since` asked the server to skip the very event that was lost
— and this branch removed the Resync that used to be the fallback.
module_updates emits a frame per apt line, which is the rate that overflows.
tseq now moves only for a delivered event and freezes at the first drop, since
it is a promise that everything through it arrived; the next snapshot clears
it. A dropped snapshot no longer adopts its own tseq either, or the resume
would resume onto nothing.

Also: one action at a time is now an invariant of the model rather than a
property of the disabled buttons — with a dist-upgrade the window where a
second run shares the first one's feed is half an hour, not a blink. `skipped`
still finishes the button, because it is usually core declining to queue a
duplicate, but the run's reason goes into the feed: the same status is how a
gate that will never open ends, and that one means the work silently did not
happen. And the feed keeps only the last 500 lines, the window core itself
keeps — the card renders one Text per line in a non-lazy Column and a run's
text is unbounded.

The unit tests did not compile on this branch: push(ProgressEntity) made
`json.decodeFromString(...)` ambiguous at four call sites, and only
assembleDebug had been run. Fixed, with tests for the rollup, the phases and
the cap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Owner

All seven addressed in a2b6dec, plus the two notes. Where the fix differs from the one suggested, why:

Blockers

1. PULL is not a layer — dropped from layers, and it is in no bucket at all now (resources already excluded it), which is where update_feed.svelte.ts puts it. The aggregate mean and the "already present or queued" count are both over real layers again.

2. Null snapshot / rejected subscriptiondecodeFromString<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 into LiveClient: a new terminal LiveEvent.Rejected is emitted before the sub is dropped, since a caller waiting on the topic for something to finish otherwise waits forever. startLogFollows ignores 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 single endRun that every terminal path goes through, so no second finishRun when a reconnect re-snapshots a run that already ended.

Should fix

4. phase carried but never read — filtering sized() 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.ts does not hit that because downloaded prefers the rollup and only falls back to the layer sum). So PullSummary is now parsed off the kind: "pull" entity and transferred/transferTotal read it, with the download-phase layer sum as the fallback for a server too old to send the rollup or phase — 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?.jsonObjectas? JsonObject / as? JsonPrimitive.

6. Two actions in flightrunAction and openAction return 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 frameSub.emit reports whether trySend landed. tseq advances 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 own tseq either: resuming from it would resume onto no baseline at all.

Notes

  • skipped — still finishes the button, but the run's reason is 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.ts MAX_LINES), in ProgressFeed rather 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.kt did not compile on this branch: the new push(ProgressEntity) overload made json.decodeFromString(...) ambiguous at four call sites. Only assembleDebug had been run, and CI runs testReleaseUnitTest — so this would have gone red on merge. Call sites made explicit; ./gradlew testDebugUnitTest is green (14 in ProgressFeedTest, including new ones for the rollup, the two phases and the line cap), assembleDebug green.

Still not exercised against a live server — module_updates remains the companion PR.

All seven addressed in `a2b6dec`, plus the two notes. Where the fix differs from the one suggested, why: ## Blockers **1. `PULL` is not a layer** — dropped from `layers`, and it is in no bucket at all now (`resources` already excluded it), which is where `update_feed.svelte.ts` puts 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 into `LiveClient`: a new terminal `LiveEvent.Rejected` is emitted before the sub is dropped, since a caller waiting on the topic for something to finish otherwise waits forever. `startLogFollows` ignores 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 single `endRun` that every terminal path goes through, so no second `finishRun` when a reconnect re-snapshots a run that already ended. ## Should fix **4. `phase` carried but never read** — filtering `sized()` 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.ts` does not hit that because `downloaded` prefers the rollup and only falls back to the layer sum). So `PullSummary` is now parsed off the `kind: "pull"` entity and `transferred`/`transferTotal` read it, with the download-phase layer sum as the fallback for a server too old to send the rollup or `phase` — 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** — `runAction` and `openAction` return 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.emit` reports whether `trySend` landed. `tseq` advances 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 own `tseq` either: resuming from it would resume onto no baseline at all. ## Notes - **`skipped`** — still finishes the button, but the run's `reason` is 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.ts` `MAX_LINES`), in `ProgressFeed` rather 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.kt` did not compile on this branch: the new `push(ProgressEntity)` overload made `json.decodeFromString(...)` ambiguous at four call sites. Only `assembleDebug` had been run, and CI runs `testReleaseUnitTest` — so this would have gone red on merge. Call sites made explicit; `./gradlew testDebugUnitTest` is green (14 in `ProgressFeedTest`, including new ones for the rollup, the two phases and the line cap), `assembleDebug` green. Still not exercised against a live server — `module_updates` remains the companion PR.
thisilike requested changes 2026-08-12 16:25:42 +02:00
Dismissed
thisilike left a comment

Reviewed at a2b6dec, cross-checked against core f3426be (packages/sdk/tasks.ts, server/src/tasks/{types,projection,routes}.ts, events/live.ts, docker compose_progress.ts). Not built locally, so the assembleDebug claim is taken on trust.

The design holds up: response-decides-protocol, retained topic, terminal status ends the action. MAX_FEED_LINES matches core's MAX_LINES, ProgressFeed.push matches the SDK's mergeProgress field for field, RunStatus matches the SDK 1:1, coerceInputValues really is on in ApiClient, core:task:* carries no role so a non-admin does follow their own run, and skipping RefreshPolicy.gate matches that function's own doc. The null snapshot 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 off core:task:<id>. If the live channel never connects — a proxy that strips Upgrade, a blocked WS, an older server — runningAction stays 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 serves GET /api/core/tasks/:id, and LiveClient.connected is a StateFlow.

2. submitForm still 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.gap never re-baselines. It is cleared only by a snap, while subFrame keeps sending since. After one dropped frame tseq freezes 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 terminal status frame 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, unbounded resources rows, no tests for the new state machine).

Reviewed at `a2b6dec`, cross-checked against core `f3426be` (`packages/sdk/tasks.ts`, `server/src/tasks/{types,projection,routes}.ts`, `events/live.ts`, docker `compose_progress.ts`). Not built locally, so the `assembleDebug` claim is taken on trust. The design holds up: response-decides-protocol, retained topic, terminal status ends the action. `MAX_FEED_LINES` matches core's `MAX_LINES`, `ProgressFeed.push` matches the SDK's `mergeProgress` field for field, `RunStatus` matches the SDK 1:1, `coerceInputValues` really is on in `ApiClient`, `core:task:*` carries no `role` so a non-admin does follow their own run, and skipping `RefreshPolicy.gate` matches that function's own doc. The `null` snapshot 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 off `core:task:<id>`. If the live channel never connects — a proxy that strips `Upgrade`, a blocked WS, an older server — `runningAction` stays 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 serves `GET /api/core/tasks/:id`, and `LiveClient.connected` is a `StateFlow`. **2. `submitForm` still 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.gap` never re-baselines.** It is cleared only by a `snap`, while `subFrame` keeps sending `since`. After one dropped frame `tseq` freezes 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 terminal `status` frame 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`, unbounded `resources` rows, 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,
Owner

finishedAtMs is decoded and never read anywhere, and three lines of class doc (and isTerminal below) explain that UNKNOWN deliberately 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:

val isTerminal: Boolean get() = /* known terminal states */
// at the call sites:
if (run.status.isTerminal || run.finishedAtMs != null) finishRun(run)

A run core has stamped finishedAtMs on is over, whatever the app makes of its status string.

`finishedAtMs` is decoded and never read anywhere, and three lines of class doc (and `isTerminal` below) explain that `UNKNOWN` deliberately 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: ```kotlin val isTerminal: Boolean get() = /* known terminal states */ // at the call sites: if (run.status.isTerminal || run.finishedAtMs != null) finishRun(run) ``` A run core has stamped `finishedAtMs` on 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 &&
Owner

This is now an open-ended catch-all — RESOURCE, STEP, OTHER, and any string a module invents — and core keeps up to MAX_ENTITIES = 400 entities per run. UpdateProgressCard renders resources with no cap, in a plain Column inside a single LazyColumn item, on top of up to 500 Text lines from the tail. Layers get MAX_LAYER_ROWS and 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.

This is now an open-ended catch-all — RESOURCE, STEP, OTHER, and any string a module invents — and core keeps up to `MAX_ENTITIES = 400` entities per run. `UpdateProgressCard` renders `resources` with no cap, in a plain `Column` inside a single `LazyColumn` item, on top of up to 500 `Text` lines from the tail. Layers get `MAX_LAYER_ROWS` and 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 downloadLayers
get() = layers.filter { it.total != null && (it.phase == null || it.phase == "download") }
Owner

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 extract drops out of both transferred and transferTotal, so "X of ~Y" shrinks. The new test without a rollup only the download phase is summed asserts exactly that (1.0e8 of 5.0e8 while 2.0e8 has already been extracted).

No producer hits this today — docker emits the rollup and phase together — but the comment reads as though the fallback is sound, and the next module to emit phase without a rollup will inherit the bar that goes down.

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 `extract` drops out of both `transferred` and `transferTotal`, so "X of ~Y" shrinks. The new test `without a rollup only the download phase is summed` asserts exactly that (1.0e8 of 5.0e8 while 2.0e8 has already been extracted). No producer hits this today — docker emits the rollup and `phase` together — but the comment reads as though the fallback is sound, and the next module to emit `phase` without 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 = true
Owner

Blocking — gap never clears, so one dropped frame duplicates every line after it.

gap is cleared only by a snap, and subFrame keeps sending since regardless. So after a single drop:

  1. tseq freezes at the last delivered event for the rest of the connection;
  2. every subsequent event is delivered but never recorded;
  3. the next reconnect resumes from the frozen point, and the server replays everything since — all of it already on screen.

Keyed entities survive that (a repeat replaces the row), but pushLine appends, 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:

  • when gap is set, omit since/epoch in subFrame so the server answers with a fresh snapshot — ProgressFeed.of is built to consume exactly that, and it re-baselines the hole instead of papering over it;
  • do not wait for a reconnect at all. A drop is detected here, synchronously; resubscribing (unsub + sub without since) right here recovers within the same connection. As written, a dropped terminal status frame leaves the button spinning until the socket happens to break — which on a healthy connection may be never.
**Blocking — `gap` never clears, so one dropped frame duplicates every line after it.** `gap` is cleared only by a `snap`, and `subFrame` keeps sending `since` regardless. So after a single drop: 1. `tseq` freezes at the last delivered event for the rest of the connection; 2. every subsequent event is delivered but never recorded; 3. the next reconnect resumes from the frozen point, and the server replays everything since — all of it already on screen. Keyed entities survive that (a repeat replaces the row), but `pushLine` appends, 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: - when `gap` is set, omit `since`/`epoch` in `subFrame` so the server answers with a fresh snapshot — `ProgressFeed.of` is built to consume exactly that, and it re-baselines the hole instead of papering over it; - do not wait for a reconnect at all. A drop is detected here, synchronously; resubscribing (unsub + sub without `since`) right here recovers within the same connection. As written, a dropped terminal `status` frame 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 {
Owner

This is the other half of the gap problem: since is sent whenever tseq != null, including when gap is 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:

if (!sub.gap) sub.tseq?.let { put("since", it); put("epoch", sub.epoch) }

A missing since makes the server re-snapshot, which is the correct answer for a subscription that has a hole in it.

Minor, same function: when epoch is null but tseq is not (the malformed-frame path that sets tseq = 0L), this sends "epoch": null. Core compares epoch === state.epoch, so that is a guaranteed miss and a guaranteed snapshot — correct, but worth a word in the comment, since the 0L default is doing something subtler than it looks.

This is the other half of the `gap` problem: `since` is sent whenever `tseq != null`, including when `gap` is 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: ```kotlin if (!sub.gap) sub.tseq?.let { put("since", it); put("epoch", sub.epoch) } ``` A missing `since` makes the server re-snapshot, which is the correct answer for a subscription that has a hole in it. Minor, same function: when `epoch` is null but `tseq` is not (the malformed-frame path that sets `tseq = 0L`), this sends `"epoch": null`. Core compares `epoch === state.epoch`, so that is a guaranteed miss and a guaranteed snapshot — correct, but worth a word in the comment, since the `0L` default 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) return
Owner

The invariant is right, but the screen-side consequence has outgrown its original sizing. ActionsRow renders enabled = 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 and open downloads, 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:

  • only disable what actually conflicts: path actions. Navigation and open have no shared state with the run.
  • give the operator a way out. Core exposes capabilities.cancel on the projected run and POST /api/core/tasks/:id/cancel; this PR decodes neither. Even without cancel, a "stop following" that calls endRun(null) beats a page that can only be escaped by navigating away.
The invariant is right, but the screen-side consequence has outgrown its original sizing. `ActionsRow` renders `enabled = 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 and `open` downloads, 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: - only disable what actually conflicts: `path` actions. Navigation and `open` have no shared state with the run. - give the operator a way out. Core exposes `capabilities.cancel` on the projected run and `POST /api/core/tasks/:id/cancel`; this PR decodes neither. Even without cancel, a "stop following" that calls `endRun(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) {
Owner

Blocking — the same protocol switch is missing on the form path.

The comment here is right that a response either carries a runId or it does not, and that no schema field selects it. That reasoning applies verbatim to submitForm (~line 1017), which ignores runId entirely: a form whose submit route starts a host-owned run reports success and calls refetchAll() 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 reads body?.jsonObject?.get("error")?.jsonPrimitive?.content, the unguarded cast you just replaced here with as?, and it also runs inside result.fold outside the runCatching — so a valid non-object JSON error body throws in viewModelScope, which is the crash your comment above describes.

**Blocking — the same protocol switch is missing on the form path.** The comment here is right that a response either carries a `runId` or it does not, and that no schema field selects it. That reasoning applies verbatim to `submitForm` (~line 1017), which ignores `runId` entirely: a form whose submit route starts a host-owned run reports success and calls `refetchAll()` 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 reads `body?.jsonObject?.get("error")?.jsonPrimitive?.content`, the unguarded cast you just replaced here with `as?`, and it also runs inside `result.fold` outside the `runCatching` — so a valid non-object JSON error body throws in `viewModelScope`, 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) {
Owner

Blocking — no fallback and no escape when the channel is down.

After this, the only thing that clears runningAction is a terminal status off core:task:<id>. If the live channel never connects — proxy strips Upgrade, 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/:id and LiveClient.connected is a StateFlow. Either is enough:

  • poll the run over REST while connected.value == false, or
  • arm a watchdog when the follow starts and endRun("lost contact with run #$runId") if no snap has 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.

**Blocking — no fallback and no escape when the channel is down.** After this, the only thing that clears `runningAction` is a terminal status off `core:task:<id>`. If the live channel never connects — proxy strips `Upgrade`, 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/:id` and `LiveClient.connected` is a `StateFlow`. Either is enough: - poll the run over REST while `connected.value == false`, or - arm a watchdog when the follow starts and `endRun("lost contact with run #$runId")` if no `snap` has 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()}"
Owner

Nit: this renders "the run cancelled" and "the run interrupted". failed is the only status that reads as a verb — the other two need "was".

Nit: this renders "the run cancelled" and "the run interrupted". `failed` is 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) }
Owner

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 carries status and title; surfacing the non-terminal transitions (or just seeding one line from the snapshot when status == WAITING) is the difference between "nothing is happening" and "nothing is happening yet, because X".

Same branch: "step" events are dropped, and RunStep is 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.

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 carries `status` and `title`; surfacing the non-terminal transitions (or just seeding one line from the snapshot when `status == WAITING`) is the difference between "nothing is happening" and "nothing is happening *yet, because X*". Same branch: `"step"` events are dropped, and `RunStep` is 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 @@
}
@Test
fun `the pull rollup is not a layer`() {
Owner

These are good tests, but they cover the part that was already covered — pure ProgressFeed arithmetic. Everything this PR actually risks is untested:

  • LiveClient's Sub state machine: tseq adoption on snap, tseq advance only on delivered events, gap set on a dropped event, Resync suppressed for retained subs, since/epoch on the resubscribe frame. It is plain JVM code reachable by feeding handle() frames — no Android, no socket.
  • ProgressFeed.of, which is the entire retained path's baseline and has no test at all (order distinct vs associateBy last-wins, lines tail).
  • followRun's terminal paths: null snapshot, 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.

These are good tests, but they cover the part that was already covered — pure `ProgressFeed` arithmetic. Everything this PR actually risks is untested: - `LiveClient`'s `Sub` state machine: `tseq` adoption on `snap`, `tseq` advance only on delivered events, `gap` set on a dropped event, `Resync` suppressed for retained subs, `since`/`epoch` on the resubscribe frame. It is plain JVM code reachable by feeding `handle()` frames — no Android, no socket. - `ProgressFeed.of`, which is the entire retained path's baseline and has no test at all (`order` distinct vs `associateBy` last-wins, `lines` tail). - `followRun`'s terminal paths: `null` snapshot, `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.
Review of #4 was about what happens when the channel misbehaves, not
about the design. Three of those are load-bearing.

The follow no longer depends on the WebSocket alone. A watchdog gives
the retained topic 10s to deliver its snapshot — a subscribe answers
at once, so silence that long is a channel that is not delivering, not
a quiet run — and then reads the run over `GET /api/core/tasks/:id`
every 3s instead. A proxy that strips `Upgrade`, a blocked WS or a
server too old for the topic all used to leave the button pressed for
the life of the page.

A followed run also has a way out now: `capabilities.cancel` is
decoded and offers "Cancel run" when core says this viewer may, and
"Stop following" releases the screen without touching the run. Only
the actions that actually conflict are disabled while one runs —
navigation entries and `open` downloads share nothing with it, and
downloads no longer share `runningAction` at all.

`submitForm` had the exact bug this branch exists to kill: a submit
route answering `{ runId }` reported success the instant the run was
created. It follows the run instead, and does not open the
successPage, since navigating away cancels the follow. Its 4xx branch
lost the unguarded `.jsonObject` on the way.

In LiveClient, a hole in a subscription now re-baselines instead of
persisting: `since` is withheld while `gap` is set (so the server
re-snapshots), and the re-subscribe goes out immediately rather than
waiting for a reconnect that may never come on a healthy socket. If
that snapshot is dropped too, the first delivered event after it tries
again. The resume state moved to `SubState` so it can be tested
without a socket.

Smaller, from the same review:

- `finishedAtMs` ends a run whose status this build cannot name, which
  was otherwise a permanent spinner
- `waiting` and `cancelling` say so on the card; a gated run produces
  nothing at all and used to look identical to one doing work
- non-layer entity rows are capped at 8 with a "+N earlier" line —
  core keeps up to 400 per run and any module may invent a kind
- "the run was cancelled", not "the run cancelled"
- `downloadLayers`' doc says that splitting by phase stops the figure
  exceeding its total but not walking backwards

Tests: `SubState`'s whole state machine, `ProgressFeed.of`, and what
ends a run (null snapshot, unknown status, finish time, wording).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Owner

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.

followRun arms 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 into pollRun, which reads GET /api/core/tasks/:id every 3s and drives the same showRun / finishRun / endRun paths — 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.cancel is decoded (as JsonPrimitive, since the wire union is "cooperative" | false and String? would fail to decode a plain false), "Cancel run" posts to /cancel and does not release the button — cancellation is cooperative, so the run's own terminal status still ends it — and "Stop following" calls endRun(null).

Same comment's other half: ActionsRow now disables per action. path actions block on running, open actions block on a new single-valued downloading, navigation blocks on nothing. openAction no longer borrows runningAction at all, so a half-hour run and a download coexist.

2. submitForm. It reads runId off the response and follows the run, on exactly the same rule as runAction — the response decides, no schema field. It deliberately does not open the successPage when it does: navigating away cancels the follow (start clears runJob), which would restore the silent success this branch exists to remove. Noted in the PR body. The 4xx branch's body?.jsonObject?... is now (body as? JsonObject) / (… as? JsonPrimitive).

3. gap never re-baselines. Both of your changes, plus a third that fell out of them.

since/epoch are withheld while gap is 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 dropped snap clears 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.Sub holds one and subFrame(id, topic, state) is a free function. SubStateTest covers tseq adoption on snap, the tseq = 0 malformed path, advance only on delivered events, gap set, the re-subscribe and its retry, plain (which is what suppresses Resync for retained topics), and the frame's since/epoch.

Smaller ones.

  • finishedAtMs: TaskRun.isOver is status.isTerminal || finishedAtMs != null, used at both call sites. RunStatus's doc no longer claims a spinner is the price of UNKNOWN.
  • waiting: runStatus is 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 with checkpoint under what is deliberately unhandled. RunStep stays 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.
  • downloadLayers doc: says outright that splitting by phase stops the figure exceeding its total but not walking backwards, and that pull being the preferred source is why no producer hits it today.
  • "the run was cancelled" / "was interrupted": the wording moved into TaskRun.endMessage as a when per status rather than an interpolated status name, and is tested.

Tests. SubState as above; ProgressFeed.of (order distinct, last-wins by key, the 500-line tail); and what ends a run — null snapshot decoding to null rather than throwing, unknown status reading as live, finishedAtMs overriding it, and the failure wording. followRun's coroutine wiring is still untested: the ViewModel wants an Application and the container, and the module has neither Robolectric nor coroutines-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 assembleDebug and testDebugUnitTest green (67 tests). Still not exercised against a live server.

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. `followRun` arms 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 into `pollRun`, which reads `GET /api/core/tasks/:id` every 3s and drives the same `showRun` / `finishRun` / `endRun` paths — 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.cancel` is decoded (as `JsonPrimitive`, since the wire union is `"cooperative" | false` and `String?` would fail to decode a plain `false`), "Cancel run" posts to `/cancel` and does *not* release the button — cancellation is cooperative, so the run's own terminal status still ends it — and "Stop following" calls `endRun(null)`. Same comment's other half: `ActionsRow` now disables per action. `path` actions block on `running`, `open` actions block on a new single-valued `downloading`, navigation blocks on nothing. `openAction` no longer borrows `runningAction` at all, so a half-hour run and a download coexist. **2. `submitForm`.** It reads `runId` off the response and follows the run, on exactly the same rule as `runAction` — the response decides, no schema field. It deliberately does *not* open the `successPage` when it does: navigating away cancels the follow (`start` clears `runJob`), which would restore the silent success this branch exists to remove. Noted in the PR body. The 4xx branch's `body?.jsonObject?...` is now `(body as? JsonObject)` / `(… as? JsonPrimitive)`. **3. `gap` never re-baselines.** Both of your changes, plus a third that fell out of them. `since`/`epoch` are withheld while `gap` is 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 dropped `snap` clears 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.Sub` holds one and `subFrame(id, topic, state)` is a free function. `SubStateTest` covers tseq adoption on `snap`, the `tseq = 0` malformed path, advance only on delivered events, gap set, the re-subscribe and its retry, `plain` (which is what suppresses `Resync` for retained topics), and the frame's `since`/`epoch`. **Smaller ones.** - `finishedAtMs`: `TaskRun.isOver` is `status.isTerminal || finishedAtMs != null`, used at both call sites. `RunStatus`'s doc no longer claims a spinner is the price of `UNKNOWN`. - `waiting`: `runStatus` is 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 with `checkpoint` under what is deliberately unhandled. `RunStep` stays 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. - `downloadLayers` doc: says outright that splitting by phase stops the figure exceeding its total but not walking backwards, and that `pull` being the preferred source is why no producer hits it today. - "the run was cancelled" / "was interrupted": the wording moved into `TaskRun.endMessage` as a `when` per status rather than an interpolated status name, and is tested. **Tests.** `SubState` as above; `ProgressFeed.of` (order distinct, last-wins by key, the 500-line tail); and what ends a run — `null` snapshot decoding to `null` rather than throwing, unknown status reading as live, `finishedAtMs` overriding it, and the failure wording. `followRun`'s coroutine wiring is still untested: the ViewModel wants an `Application` and the container, and the module has neither Robolectric nor `coroutines-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 assembleDebug` and `testDebugUnitTest` green (67 tests). Still not exercised against a live server.
thisilike requested changes 2026-08-12 21:35:55 +02:00
Dismissed
thisilike left a comment

Reviewed at 641b860. Line refs are against that commit. ./gradlew testDebugUnitTest is green.

What lands well

  • Protocol chosen by response shape ({runId} vs progressTopic), 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.
  • downloading split out of runningAction. A 30-minute run no longer freezes every download button on the page.
  • SubState extracted so the reconnect story is testable without a socket. SubStateTest is the strongest part of the PR.

Blocking

H1 — submitForm has no "one run at a time" guard; form submit hijacks a live run

runAction got the guard (SchemaViewModel.kt:789) and ActionsRow disables posting actions on running != null. submitForm (:1092) got neitherFormCard receives only submitting = state.submittingForm (SchemaScreen.kt:272).

So while a host-owned run is followed, every form on the page stays submittable. followRun (:1182) then does runJob?.cancel() and overwrites runId / progress / runningAction. Run #1 keeps executing host-side, unobserved, outcome never reported — the exact silent lie the KDoc at :1167 says this path exists to stop.

Fix: same guard in submitForm, and feed state.runningAction into FormCard's disabled condition.

H2 — dropped-snapshot path still wedges the screen at end of run

SubState.gap clears only in onSnapshot (SubState.kt:758). Sequence:

  1. Burst overflows the 64-slot callbackFlow buffer → onEvent(delivered=false) → re-subscribe.
  2. That snapshot is also dropped → onSnapshotDropped() clears resubscribed.
  3. The retry only fires from 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 _state reaches 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-1121 fires on the first frame and never comes back. After that there is no REST fallback: socket dies, reconnect backs off to MAX_BACKOFF_MS, or the server just stops publishing — nothing ends the follow. LiveClient.connected is exposed and unused. Same fix as H2.

Medium

M1 — one unknown entity kills the whole follow. ProgressEntity requires key/id/status (UpdateProgress.kt:21-27), RunStep requires name/status (Task.kt:205). One entity missing any of them fails decodeFromString<TaskRun?>endRun("could not read the state of run #N") (:1144-1148). Contradicts the PR's own forward-compat stance three files over (UNKNOWN status, OTHER kind). steps is decoded and never rendered — it can only lose. Default those fields, or drop steps from the model.

M2 — pollRun quits 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 — pollRun polls 3s forever, background included. No backoff, no cap, viewModelScope is 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 / ev don't check ws === 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 call onSnapshot/onEvent and overwrite tseq/epoch for a subscription already resumed on the new socket — the next since then 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; push carries total/current/fraction/phase forward 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, or of() applies the same merge.

M6 — push carries phase forward but not pull. :367 carries phase specifically so counts aren't misattributed. pull gets no such treatment, so a rollup frame that omits it wipes the summary and flips transferred/transferTotal from 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.downloadTotal defaults to 0.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 one LazyColumn item. MAX_RESOURCE_ROWS = 8 was added for exactly this reason — "renders in a plain Column inside one LazyColumn item, which composes every row it is given" — and then the line ceiling went from "a handful" to 500 with no render cap. feed.services is uncapped too, against core's 400-entity budget. Also no autoscroll: the newest line pushes itself off-screen.

M9 — withheld is decoded, argued for at length, and never shown. Task.kt:22 insists "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

  • L1 canCancel requires 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.
  • L2 cancelRun clears runCancellable permanently on any refusal (:1240), transient 5xx included — no retry for the rest of the run.
  • L3 A form-started run reports its outcome into actionError, not formError — banner is nowhere near the form. And successPage is never opened even on SUCCEEDED; the KDoc justifies not navigating during the run, not after.
  • L4 stopFollowing()endRun(null) wipes any unrelated actionError already on screen.
  • L5 runId is not in SavedStateHandle. 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.
  • L6 Cancelled progressJob stays in refreshJobs (:825, :848) — one dead Job per press for the page's life.
  • L7 Between followRun and the first frame, runId != null with empty feed and null status: "Cancel run" / "Stop following" render with nothing above them.
  • L8 pullSettled requires layers.isNotEmpty() — an all-cached pull (rollup, zero LAYER entities) reads "Pulling images" forever.
  • L9 New run code uses _state.value = _state.value.copy(…) throughout, while :1131 documents exactly why _state.update {} is needed. Main-thread-only today, but the hazard is already written down in the same file.
  • L10 SubState has no thread-safety note. Every access is under LiveClient.lock; nothing in the class says so, and it's internal.

Tests

SubStateTest covers the right cases — "one re-subscribe per hole" and "a re-subscribe whose snapshot also dropped is sent again" are the two that matter. TaskRunTest covers the unknown-status escape hatch properly.

Gap: followRun's state machine — watchdog, Rejected, double-follow, endRun idempotence — 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. SubStateTest structurally cannot reach H2: the retry trigger sits in LiveClient's frame loop, not in SubState.

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.kt and followRun now 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.

Reviewed at `641b860`. Line refs are against that commit. `./gradlew testDebugUnitTest` is green. ## What lands well - Protocol chosen by **response shape** (`{runId}` vs `progressTopic`), 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. - `downloading` split out of `runningAction`. A 30-minute run no longer freezes every download button on the page. - `SubState` extracted so the reconnect story is testable without a socket. `SubStateTest` is the strongest part of the PR. ## Blocking ### H1 — `submitForm` has no "one run at a time" guard; form submit hijacks a live run `runAction` got the guard (`SchemaViewModel.kt:789`) and `ActionsRow` disables posting actions on `running != null`. `submitForm` (`:1092`) got **neither** — `FormCard` receives only `submitting = state.submittingForm` (`SchemaScreen.kt:272`). So while a host-owned run is followed, every form on the page stays submittable. `followRun` (`:1182`) then does `runJob?.cancel()` and overwrites `runId` / `progress` / `runningAction`. Run #1 keeps executing host-side, unobserved, outcome never reported — the exact silent lie the KDoc at `:1167` says this path exists to stop. Fix: same guard in `submitForm`, and feed `state.runningAction` into `FormCard`'s disabled condition. ### H2 — dropped-snapshot path still wedges the screen at end of run `SubState.gap` clears only in `onSnapshot` (`SubState.kt:758`). Sequence: 1. Burst overflows the 64-slot `callbackFlow` buffer → `onEvent(delivered=false)` → re-subscribe. 2. That snapshot is **also** dropped → `onSnapshotDropped()` clears `resubscribed`. 3. The retry only fires from `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 `_state` reaches 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-1121` fires on the first frame and never comes back. After that there is no REST fallback: socket dies, reconnect backs off to `MAX_BACKOFF_MS`, or the server just stops publishing — nothing ends the follow. `LiveClient.connected` is exposed and unused. Same fix as H2. ## Medium **M1 — one unknown entity kills the whole follow.** `ProgressEntity` requires `key`/`id`/`status` (`UpdateProgress.kt:21-27`), `RunStep` requires `name`/`status` (`Task.kt:205`). One entity missing any of them fails `decodeFromString<TaskRun?>` → `endRun("could not read the state of run #N")` (`:1144-1148`). Contradicts the PR's own forward-compat stance three files over (`UNKNOWN` status, `OTHER` kind). `steps` is decoded and **never rendered** — it can only lose. Default those fields, or drop `steps` from the model. **M2 — `pollRun` quits 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 — `pollRun` polls 3s forever, background included.** No backoff, no cap, `viewModelScope` is 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` / `ev` don't check `ws === 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 call `onSnapshot`/`onEvent` and overwrite `tseq`/`epoch` for a subscription already resumed on the new socket — the next `since` then 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; `push` carries `total`/`current`/`fraction`/`phase` forward 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, or `of()` applies the same merge. **M6 — `push` carries `phase` forward but not `pull`.** `:367` carries `phase` specifically so counts aren't misattributed. `pull` gets no such treatment, so a rollup frame that omits it wipes the summary and flips `transferred`/`transferTotal` from 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.downloadTotal` defaults to `0.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 one `LazyColumn` item. `MAX_RESOURCE_ROWS = 8` was added for exactly this reason — "renders in a plain `Column` inside one `LazyColumn` item, which composes every row it is given" — and then the line ceiling went from "a handful" to 500 with no render cap. `feed.services` is uncapped too, against core's 400-entity budget. Also no autoscroll: the newest line pushes itself off-screen. **M9 — `withheld` is decoded, argued for at length, and never shown.** `Task.kt:22` insists "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 - **L1** `canCancel` requires 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. - **L2** `cancelRun` clears `runCancellable` permanently on any refusal (`:1240`), transient 5xx included — no retry for the rest of the run. - **L3** A form-started run reports its outcome into `actionError`, not `formError` — banner is nowhere near the form. And `successPage` is never opened even on `SUCCEEDED`; the KDoc justifies not navigating *during* the run, not after. - **L4** `stopFollowing()` → `endRun(null)` wipes any unrelated `actionError` already on screen. - **L5** `runId` is not in `SavedStateHandle`. 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. - **L6** Cancelled `progressJob` stays in `refreshJobs` (`:825`, `:848`) — one dead Job per press for the page's life. - **L7** Between `followRun` and the first frame, `runId != null` with empty feed and null status: "Cancel run" / "Stop following" render with nothing above them. - **L8** `pullSettled` requires `layers.isNotEmpty()` — an all-cached pull (rollup, zero LAYER entities) reads "Pulling images" forever. - **L9** New run code uses `_state.value = _state.value.copy(…)` throughout, while `:1131` documents exactly why `_state.update {}` is needed. Main-thread-only today, but the hazard is already written down in the same file. - **L10** `SubState` has no thread-safety note. Every access is under `LiveClient.lock`; nothing in the class says so, and it's `internal`. ## Tests `SubStateTest` covers the right cases — "one re-subscribe per hole" and "a re-subscribe whose snapshot also dropped is sent again" are the two that matter. `TaskRunTest` covers the unknown-status escape hatch properly. Gap: `followRun`'s state machine — watchdog, `Rejected`, double-follow, `endRun` idempotence — 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. `SubStateTest` structurally cannot reach H2: the retry trigger sits in `LiveClient`'s frame loop, not in `SubState`. ## 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.kt` and `followRun` now 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>
Author
Owner

All findings from the third review addressed in 4950da3.

Blocking

  • H1submitForm now has the same one-run-at-a-time guard as runAction (model invariant), and FormCard gets runInFlight so every submit button on the page disables while a run is followed.
  • H2/H3 — one mechanism closes both: the silence watchdog is re-armed on every frame instead of being cancelled by the first one. After FOLLOW_SILENCE_MS of channel silence it reconciles the run over GET /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

  • M1ProgressEntity and RunStep default every field; one degraded entity in a snapshot degrades that entity, not the follow. steps stays decoded (it still arrives inside the snapshot either way).
  • M2 — the probe tolerates 3 consecutive failures before declaring loss; a Wi-Fi→LTE handover is one.
  • M3 — probe cadence backs off 10s→20s→30s and skips the network while ProcessLifecycleOwner says the app is not STARTED (the timer keeps running, the radio stays asleep).
  • M4snap/ev now check ws === socket before emitting or touching SubState, same as every other handler.
  • M5ProgressFeed.of(run, prev) merges each snapshot entity against what the screen already knew, through the same merge push uses; entities absent from the snapshot are still dropped (the snapshot stays the whole truth).
  • M6 — that merge now carries pull forward alongside phase.
  • M7sizedPull gates the rollup on downloadTotal > 0; null and zero both mean "nothing to read yet, use the layer sum" — numerator and denominator switch sources together.
  • M8 — the card renders a 30-line tail (+ 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.
  • M9 — the card shows "Not shown for your role: …" from withheld, which the snapshot now carries into the feed. README updated to match.

Low

  • L1 — any mechanism name enables cancel; only false/absent deny.
  • L2 — a refused cancel keeps the button; if core really means never, the next status frame's capabilities clear it.
  • L3 — form-started runs report into formError, and successPage opens after SUCCEEDED (deferred, not dropped — the KDoc now says so).
  • L4endRun(null) preserves existing banners.
  • L5 — narrowed the claim: resume holds for the ViewModel's life, a process death orphans the follow. (Chose the doc fix over SavedStateHandle; happy to add the restore as a follow-up if you want it.)
  • L6progressJob is its own field, cancelled on page change like runJob.
  • L7 — the card renders whenever runId != null, with a "Following the run — waiting for its first report" line under it, so the buttons sit on something.
  • L8 — with zero layer entities, pullSettled follows the rollup entity's own state.
  • L9 — all run-path writes go through _state.update {}.
  • L10SubState documents the lock contract.

Tests

8 new: any-mechanism cancel, degraded entity/step decode, withheld into the feed, re-snapshot merge carry-forward (and that absent entities still drop), pull carried across a rollup frame that omits it, zero-total rollup deferring to the layer sum, all-cached pull settling. 75 total, green; assembleDebug green.

The followRun coroutine 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.

All findings from the third review addressed in `4950da3`. ## Blocking - **H1** — `submitForm` now has the same one-run-at-a-time guard as `runAction` (model invariant), and `FormCard` gets `runInFlight` so every submit button on the page disables while a run is followed. - **H2/H3** — one mechanism closes both: the silence watchdog is **re-armed on every frame** instead of being cancelled by the first one. After `FOLLOW_SILENCE_MS` of channel silence it reconciles the run over `GET /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 - **M1** — `ProgressEntity` and `RunStep` default every field; one degraded entity in a snapshot degrades that entity, not the follow. `steps` stays decoded (it still arrives inside the snapshot either way). - **M2** — the probe tolerates 3 consecutive failures before declaring loss; a Wi-Fi→LTE handover is one. - **M3** — probe cadence backs off 10s→20s→30s and skips the network while `ProcessLifecycleOwner` says the app is not STARTED (the timer keeps running, the radio stays asleep). - **M4** — `snap`/`ev` now check `ws === socket` before emitting or touching `SubState`, same as every other handler. - **M5** — `ProgressFeed.of(run, prev)` merges each snapshot entity against what the screen already knew, through the same merge `push` uses; entities absent from the snapshot are still dropped (the snapshot stays the whole truth). - **M6** — that merge now carries `pull` forward alongside `phase`. - **M7** — `sizedPull` gates the rollup on `downloadTotal > 0`; null and zero both mean "nothing to read yet, use the layer sum" — numerator and denominator switch sources together. - **M8** — the card renders a 30-line tail (`+ 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. - **M9** — the card shows "Not shown for your role: …" from `withheld`, which the snapshot now carries into the feed. README updated to match. ## Low - **L1** — any mechanism name enables cancel; only `false`/absent deny. - **L2** — a refused cancel keeps the button; if core really means never, the next status frame's capabilities clear it. - **L3** — form-started runs report into `formError`, and `successPage` opens after `SUCCEEDED` (deferred, not dropped — the KDoc now says so). - **L4** — `endRun(null)` preserves existing banners. - **L5** — narrowed the claim: resume holds for the ViewModel's life, a process death orphans the follow. (Chose the doc fix over `SavedStateHandle`; happy to add the restore as a follow-up if you want it.) - **L6** — `progressJob` is its own field, cancelled on page change like `runJob`. - **L7** — the card renders whenever `runId != null`, with a "Following the run — waiting for its first report" line under it, so the buttons sit on something. - **L8** — with zero layer entities, `pullSettled` follows the rollup entity's own state. - **L9** — all run-path writes go through `_state.update {}`. - **L10** — `SubState` documents the lock contract. ## Tests 8 new: any-mechanism cancel, degraded entity/step decode, `withheld` into the feed, re-snapshot merge carry-forward (and that absent entities still drop), `pull` carried across a rollup frame that omits it, zero-total rollup deferring to the layer sum, all-cached pull settling. 75 total, green; `assembleDebug` green. The `followRun` coroutine 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.
thisilike left a comment

Re-review at 4950da3. ./gradlew testDebugUnitTest green.

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

  • H1submitForm:1218 has the guard, FormCard.runInFlight (FormCard.kt:308, wired at SchemaScreen.kt:271) disables the button. Model invariant and screen, same as the actions.
  • H2/H3 — the watchdog is re-armed on every frame (:1018) instead of cancelled once, so a socket that dies mid-run, and a burst that swallows the terminal status, both reconcile over REST within FOLLOW_SILENCE_MS. Backoff to 30s and the ProcessLifecycleOwner foreground 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.
  • M1 RunStep/ProgressEntity fully defaulted, with the "wrong row beats a dead screen" trade written down. M2 three-failure budget. M4 ws === socket on both snap and ev, including the onSnapshotDropped path. M5/M6 merge extracted and applied to of(), pull carried forward with phase. M7 sizedPull. M8 MAX_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-prev in ProgressFeed.of with "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 at TaskRunTest pins both halves.

New

N1 — runCatching in probe() swallows CancellationException. :963. Every frame calls armWatchdog()silence?.cancel() (:996), which cancels an in-flight probe. taskRun's withContext throws CancellationException, runCatching catches it, and :967 counts it as a network failure.

It can't currently reach the threshold — the collector sets probeFailures = 0 at :1016 before 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 }, or currentCoroutineContext().ensureActive() after the runCatching.

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 — lines wholesale, and runStatus can regress (live "cancelling" → stale "running"). Self-corrects on the next frame, and terminal is safe (endRun cancels the probe's parent), so it's a flicker rather than a wedge. finishedAtMs/a monotonic stamp, or just skipping showRun when 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 _state copy per line already dominates, so this isn't the bottleneck — but a lastFrameAtMs timestamp checked by one long-lived loop gets the same behavior with no allocation.

N4 — runOnSuccess holds a composable's lambda in the ViewModel for the life of the run. :271, assigned at :1310 from the onSuccess closure SchemaScreen recreates on every recomposition. It captures onNavigateSub, and it's invoked minutes later at :1157. Storing the interpolated successPage in SchemaUiState as 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:141 vs :171. The resources comment argues the tail is right because the newest rows are the ones still moving; services get take() 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 key quietly eats the legacy text fallback. With ProgressEntity.key now defaulted, a legacy progressTopic frame carrying a malformed progress object decodes instead of throwing, so runAction's "fall back to the raw msg as 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. followRun grew probe(), 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/TaskRunTest continue 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 versionName bump in this commit. Reasonable while 0.31.0 is unreleased on the branch — flagging only so it's a decision rather than an oversight.

Re-review at `4950da3`. `./gradlew testDebugUnitTest` green. **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 - **H1** — `submitForm:1218` has the guard, `FormCard.runInFlight` (`FormCard.kt:308`, wired at `SchemaScreen.kt:271`) disables the button. Model invariant *and* screen, same as the actions. - **H2/H3** — the watchdog is re-armed on every frame (`:1018`) instead of cancelled once, so a socket that dies mid-run, and a burst that swallows the terminal status, both reconcile over REST within `FOLLOW_SILENCE_MS`. Backoff to 30s and the `ProcessLifecycleOwner` foreground 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. - **M1** `RunStep`/`ProgressEntity` fully defaulted, with the "wrong row beats a dead screen" trade written down. **M2** three-failure budget. **M4** `ws === socket` on both `snap` and `ev`, including the `onSnapshotDropped` path. **M5/M6** `merge` extracted and applied to `of()`, `pull` carried forward with `phase`. **M7** `sizedPull`. **M8** `MAX_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-`prev` in `ProgressFeed.of` with "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 at `TaskRunTest` pins both halves. ## New **N1 — `runCatching` in `probe()` swallows `CancellationException`.** `:963`. Every frame calls `armWatchdog()` → `silence?.cancel()` (`:996`), which cancels an in-flight probe. `taskRun`'s `withContext` throws `CancellationException`, `runCatching` catches it, and `:967` counts it as a network failure. It can't currently reach the threshold — the collector sets `probeFailures = 0` at `:1016` before 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 }`, or `currentCoroutineContext().ensureActive()` after the `runCatching`. **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 — `lines` wholesale, and `runStatus` can regress (live "cancelling" → stale "running"). Self-corrects on the next frame, and terminal is safe (`endRun` cancels the probe's parent), so it's a flicker rather than a wedge. `finishedAtMs`/a monotonic stamp, or just skipping `showRun` when 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 `_state` copy per line already dominates, so this isn't the bottleneck — but a `lastFrameAtMs` timestamp checked by one long-lived loop gets the same behavior with no allocation. **N4 — `runOnSuccess` holds a composable's lambda in the ViewModel for the life of the run.** `:271`, assigned at `:1310` from the `onSuccess` closure `SchemaScreen` recreates on every recomposition. It captures `onNavigateSub`, and it's invoked minutes later at `:1157`. Storing the interpolated `successPage` in `SchemaUiState` as 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:141` vs `:171`. The resources comment argues the tail is right because the newest rows are the ones still moving; services get `take()` 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 `key` quietly eats the legacy text fallback.** With `ProgressEntity.key` now defaulted, a legacy `progressTopic` frame carrying a malformed `progress` object decodes instead of throwing, so `runAction`'s "fall back to the raw `msg` as 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. `followRun` grew `probe()`, `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`/`TaskRunTest` continue 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 `versionName` bump in this commit. Reasonable while 0.31.0 is unreleased on the branch — flagging only so it's a decision rather than an oversight.
thisilike requested changes 2026-08-12 22:07:57 +02:00
Dismissed
thisilike left a comment

Full review of the PR at 4950da3 — whole diff against 473ef61, not just the last commit. Supersedes the two earlier passes; this one stands alone. ./gradlew testDebugUnitTest green.

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:

  • The response picks the protocol. {runId} vs progressTopic, 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.
  • The retained topic is the whole contract. Snapshot replaces, deltas append, reconnect resumes — and 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.
  • SubState extracted. The reconnect story is the whole correctness story and it's the one piece testable without a socket. SubStateTest is the strongest file in the PR.
  • REST twin as the floor. A run stays observable when WebSockets don't get through. Right instinct.

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:1218 guards on runningAction != null, and FormCard.runInFlight (SchemaScreen.kt:271) disables the forms while an action runs.
  • runAction:826 guards on runningAction != null — but a form submit sets submittingForm, not runningAction. runningAction is only set once the response comes back carrying a runId (: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 a runId; the second followRun does runJob?.cancel() and overwrites runId / 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:519 calls runAction with 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 != null used by runAction, submitForm, ActionsRow and FormCard makes the invariant one thing instead of four.

B2 — a wedged legacy progressTopic run now freezes the forms too

runInFlight = state.runningAction != null is new, and the legacy path sets runningAction as well.

A progressTopic action 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 — runningAction never 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.runId is 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.id is the last required field, and a status delta that omits it is dropped silently

The last commit correctly defaulted every field of RunStep and ProgressEntity, with the reasoning written down: one required field a future core omits fails the whole decode. TaskRun.id (Task.kt:28) is still required, and TaskEvent.run is a TaskRun.

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 — fails decodeFromString<TaskEvent>, and :1051 discards it:

val ev = runCatching { client.json.decodeFromString<TaskEvent>(event.data) }
    .getOrNull() ?: return@collect

No log, no state change. The terminal frame vanishes and the 10s watchdog is the only thing that eventually notices. Default id to 0L like 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 — runCatching in probe() swallows CancellationException. :963. Every frame calls armWatchdog()silence?.cancel() (:996), cancelling an in-flight probe; taskRun's withContext throws, runCatching catches it, :967 counts it as a network failure. It can't reach the threshold today — the collector zeroes probeFailures at :1016 before 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 — lines wholesale, and runStatus can regress (live "cancelling" → stale "running"). Self-correcting, and terminal is safe because endRun cancels the probe's parent, so it's a flicker not a wedge. Skipping showRun when a frame landed after the probe started closes it.

S3 — runOnSuccess parks a composable's lambda in the ViewModel for the life of the run. :271, assigned at :1310 from the closure SchemaScreen rebuilds on every recomposition, invoked minutes later at :1157. It captures onNavigateSub. Put the interpolated successPage in SchemaUiState as a one-shot event and let the screen navigate; the ViewModel shouldn't hold nav callbacks.

S4 — a refused refresh topic dies silently. startRefreshTriggers:382 collects policy.gate(topic) and calls refetchAll() on any event, LiveEvent.Rejected included — so a rejection fires one refetch and the page then never auto-refreshes again, with nothing said. The log-follow path at :462 at least documents the choice ("costs freshness, not correctness"); a page's refresh trigger is a stronger claim to lose quietly. Now that Rejected exists, 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 _state copy per line dominates, so this isn't the bottleneck, but a lastFrameAtMs timestamp checked by one long-lived loop is the same behavior with none of it.

S6 — a defaulted key eats the legacy text fallback. With ProgressEntity.key defaulted, a legacy frame carrying a malformed progress object now decodes instead of throwing, so runAction's "fall back to the raw msg as 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

  • Services take the head, resources take the tail. UpdateProgressCard.kt:141 vs :171. The resources comment argues the tail is correct because the newest rows are the ones still moving; services get take() with "same budget as resources". And the overflow labels disagree about which end was cut — "+ N more" vs "+ N earlier".
  • withheld renders raw wire field names ("entities", "lines") straight into Not shown for your role:. A small map to operator-facing words would finish the thought the field exists for.
  • "Following the run — waiting for its first report" (UpdateProgressCard.kt:99) has no trailing Spacer, unlike every sibling block in the card.
  • Cancel run has no confirmation while the schema carries a whole typed-phrase confirm mechanism. Probably right — cooperative cancel is cheap and re-runnable — but it's the one destructive-ish control in the PR that bypasses the brake, so worth being a decision.
  • Eight TaskRun fields 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

SubStateTest and TaskRunTest cover 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: followRun holds probe(), armWatchdog(), the failure budget, the foreground check, the form-run success routing and the endRun banner 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 — but runTest plus a fake ApiClient/LiveClient would have caught at least three of the findings above, and this is the file where a silent wrong answer costs the most.


Housekeeping

versionName 0.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.kt and followRun now carry design-doc paragraphs that will drift faster than the code under them. The last round already produced one instance (a doc asserting withheld handling 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 in RunStep's KDoc while TaskRun.id quietly doesn't follow it.

Full review of the PR at `4950da3` — whole diff against `473ef61`, not just the last commit. Supersedes the two earlier passes; this one stands alone. `./gradlew testDebugUnitTest` green. 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: - **The response picks the protocol.** `{runId}` vs `progressTopic`, 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. - **The retained topic is the whole contract.** Snapshot replaces, deltas append, reconnect resumes — and `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. - **`SubState` extracted.** The reconnect story is the whole correctness story and it's the one piece testable without a socket. `SubStateTest` is the strongest file in the PR. - **REST twin as the floor.** A run stays observable when WebSockets don't get through. Right instinct. --- ## 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:1218` guards on `runningAction != null`, and `FormCard.runInFlight` (`SchemaScreen.kt:271`) disables the forms while an action runs. ✅ - `runAction:826` guards on `runningAction != null` — but a form submit sets **`submittingForm`**, not `runningAction`. `runningAction` is only set once the response comes back carrying a `runId` (`: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 a `runId`; the second `followRun` does `runJob?.cancel()` and overwrites `runId` / `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:519` calls `runAction` with 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 != null` used by `runAction`, `submitForm`, `ActionsRow` and `FormCard` makes the invariant one thing instead of four. ### B2 — a wedged legacy `progressTopic` run now freezes the forms too `runInFlight = state.runningAction != null` is new, and the legacy path sets `runningAction` as well. A `progressTopic` action 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 — `runningAction` never 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.runId` is 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.id` is the last required field, and a status delta that omits it is dropped silently The last commit correctly defaulted every field of `RunStep` and `ProgressEntity`, with the reasoning written down: one required field a future core omits fails the *whole* decode. `TaskRun.id` (`Task.kt:28`) is still required, and `TaskEvent.run` is a `TaskRun`. 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 — fails `decodeFromString<TaskEvent>`, and `:1051` discards it: ```kotlin val ev = runCatching { client.json.decodeFromString<TaskEvent>(event.data) } .getOrNull() ?: return@collect ``` No log, no state change. The terminal frame vanishes and the 10s watchdog is the only thing that eventually notices. Default `id` to `0L` like 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 — `runCatching` in `probe()` swallows `CancellationException`.** `:963`. Every frame calls `armWatchdog()` → `silence?.cancel()` (`:996`), cancelling an in-flight probe; `taskRun`'s `withContext` throws, `runCatching` catches it, `:967` counts it as a network failure. It can't reach the threshold today — the collector zeroes `probeFailures` at `:1016` before 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 — `lines` wholesale, and `runStatus` can regress (live "cancelling" → stale "running"). Self-correcting, and terminal is safe because `endRun` cancels the probe's parent, so it's a flicker not a wedge. Skipping `showRun` when a frame landed after the probe started closes it. **S3 — `runOnSuccess` parks a composable's lambda in the ViewModel for the life of the run.** `:271`, assigned at `:1310` from the closure `SchemaScreen` rebuilds on every recomposition, invoked minutes later at `:1157`. It captures `onNavigateSub`. Put the interpolated `successPage` in `SchemaUiState` as a one-shot event and let the screen navigate; the ViewModel shouldn't hold nav callbacks. **S4 — a refused refresh topic dies silently.** `startRefreshTriggers:382` collects `policy.gate(topic)` and calls `refetchAll()` on *any* event, `LiveEvent.Rejected` included — so a rejection fires one refetch and the page then never auto-refreshes again, with nothing said. The log-follow path at `:462` at least documents the choice ("costs freshness, not correctness"); a page's refresh trigger is a stronger claim to lose quietly. Now that `Rejected` exists, 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 `_state` copy per line dominates, so this isn't the bottleneck, but a `lastFrameAtMs` timestamp checked by one long-lived loop is the same behavior with none of it. **S6 — a defaulted `key` eats the legacy text fallback.** With `ProgressEntity.key` defaulted, a legacy frame carrying a malformed `progress` object now decodes instead of throwing, so `runAction`'s "fall back to the raw `msg` as 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 - **Services take the head, resources take the tail.** `UpdateProgressCard.kt:141` vs `:171`. The resources comment argues the tail is correct because the newest rows are the ones still moving; services get `take()` with "same budget as resources". And the overflow labels disagree about which end was cut — "+ N more" vs "+ N earlier". - **`withheld` renders raw wire field names** ("entities", "lines") straight into `Not shown for your role:`. A small map to operator-facing words would finish the thought the field exists for. - **"Following the run — waiting for its first report"** (`UpdateProgressCard.kt:99`) has no trailing `Spacer`, unlike every sibling block in the card. - **Cancel run has no confirmation** while the schema carries a whole typed-phrase confirm mechanism. Probably right — cooperative cancel is cheap and re-runnable — but it's the one destructive-ish control in the PR that bypasses the brake, so worth being a decision. - **Eight `TaskRun` fields 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 `SubStateTest` and `TaskRunTest` cover 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: `followRun` holds `probe()`, `armWatchdog()`, the failure budget, the foreground check, the form-run success routing and the `endRun` banner 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 — but `runTest` plus a fake `ApiClient`/`LiveClient` would have caught at least three of the findings above, and this is the file where a silent wrong answer costs the most. --- ## Housekeeping `versionName` 0.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.kt` and `followRun` now carry design-doc paragraphs that will drift faster than the code under them. The last round already produced one instance (a doc asserting `withheld` handling 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 in `RunStep`'s KDoc while `TaskRun.id` quietly doesn't follow it.
Fourth review round (N1-N6), all in the follow's supporting cast:

- probe() rethrows CancellationException instead of counting its own
  cancellation as a network failure and dirtying the retry budget (N1)
- a probe whose GET raced a live frame discards its response: the channel
  is ahead of it, and showing it rolled lines and status backwards (N2)
- the silence watchdog is one long-lived loop re-baselined by a timestamp,
  not a Job cancelled and relaunched per frame - that was an allocation
  per apt log line, on Main (N3)
- a form's successPage travels as one-shot SchemaUiState.openPage that the
  screen navigates on and acknowledges; the ViewModel no longer holds a
  recomposition-recreated composable lambda for the life of a run (N4)
- services overflow like resources now: tail kept, "+ N earlier" - head
  here and tail there had the two lines disagreeing about which end got
  cut (N5)
- a keyless entity on the legacy progressTopic stream demotes back to its
  raw msg text: the defaulted key exists for the snapshot bulk decode and
  was silently eating the text fallback here (N6)

./gradlew testDebugUnitTest green (76), assembleDebug green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

N1–N6 addressed in eded824.

  • N1probe() rethrows CancellationException (.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.
  • N2 — a probe records the frame counter before its GET and discards the response if the counter moved: a frame landing mid-round-trip means the channel is alive and already ahead of the REST read. If the run really went terminal, the terminal frame or the next probe says so — the stale snapshot never touches the screen.
  • N3 — your lastFrameAtMs suggestion, 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 per FOLLOW_SILENCE_MS no 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.
  • N4 — the lambda is gone. The form's interpolated successPage is held as a plain String for the run's life and surfaces as one-shot SchemaUiState.openPage; the screen navigates in a LaunchedEffect and acknowledges via pageOpened() before navigating, so a recomposition on the way out cannot fire it twice. submitForm lost its callback parameter entirely — the immediate (non-run) path goes through the same event.
  • N5 — services take the tail with + N earlier, same budget, same end, same wording as resources. The comment now argues it once for both.
  • N6 — a decoded event whose entity has an empty key demotes back to its raw msg on 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:

  • ViewModel test gap — unchanged this round, and it grew again (the probe loop, the staleness check). Standing offer from last round holds: a Robolectric-or-seam fixture as its own PR, since it's a build-dependency decision, not a test-writing one. Say the word.
  • versionName — staying at 0.31.0: the branch is unreleased, so these fixes fold into the bump this PR already carries. Decision, not oversight.

./gradlew testDebugUnitTest green (76), assembleDebug green. Still not exercised against a live server — module_updates remains the companion PR.

N1–N6 addressed in `eded824`. - **N1** — `probe()` rethrows `CancellationException` (`.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. - **N2** — a probe records the frame counter before its GET and discards the response if the counter moved: a frame landing mid-round-trip means the channel is alive and already ahead of the REST read. If the run really went terminal, the terminal frame or the next probe says so — the stale snapshot never touches the screen. - **N3** — your `lastFrameAtMs` suggestion, 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 per `FOLLOW_SILENCE_MS` no 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. - **N4** — the lambda is gone. The form's interpolated successPage is held as a plain `String` for the run's life and surfaces as one-shot `SchemaUiState.openPage`; the screen navigates in a `LaunchedEffect` and acknowledges via `pageOpened()` before navigating, so a recomposition on the way out cannot fire it twice. `submitForm` lost its callback parameter entirely — the immediate (non-run) path goes through the same event. - **N5** — services take the tail with `+ N earlier`, same budget, same end, same wording as resources. The comment now argues it once for both. - **N6** — a decoded event whose entity has an empty `key` demotes back to its raw `msg` on 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:** - **ViewModel test gap** — unchanged this round, and it grew again (the probe loop, the staleness check). Standing offer from last round holds: a Robolectric-or-seam fixture as its own PR, since it's a build-dependency decision, not a test-writing one. Say the word. - **versionName** — staying at 0.31.0: the branch is unreleased, so these fixes fold into the bump this PR already carries. Decision, not oversight. `./gradlew testDebugUnitTest` green (76), `assembleDebug` green. Still not exercised against a live server — `module_updates` remains the companion PR.
thisilike requested changes 2026-08-12 22:16:17 +02:00
Dismissed
thisilike left a comment

Review at eded824. ./gradlew testDebugUnitTest green.

fix(schema): the probe's blind spots, and nav state out of the ViewModel closes 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

  • S1 .onFailure { if (it is CancellationException) throw it } (:983) — and it sits before the failure accounting, so a cancelled probe no longer dirties the budget.
  • S2 the frames snapshot (: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.
  • S3 openPage as one-shot state, acknowledged via pageOpened() before onNavigateSub (SchemaScreen.kt:125-131), and the LaunchedEffect is above the loading/error early-returns so it can't be composed away. Both details are the ones that usually get this pattern wrong.
  • S5 the single loop with SystemClock.elapsedRealtime() — monotonic, right clock, no per-frame allocation.
  • S6 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. :985 returns true for the stale case, and the loop then unconditionally runs baseline = elapsedRealtime() / delayMs = min(delayMs * 2, …) (:1045-1046). Because baseline is now ahead of lastFrameAtMs, the lastFrameAtMs > baseline reset at :1026 can'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"). probeFailures does get reset by the collector, so only the cadence is wrong. Distinguish stale from healthy-still-running in the return, or set baseline = lastFrameAtMs; delayMs = FOLLOW_SILENCE_MS on the stale path.

N2 — navigating away doesn't orphan the follow, it hides it — and the KDoc says otherwise. :935 claims "navigating to another page orphans the follow". MainScaffold.kt:335 puts every module page on one composable("module/{name}?path={path}") route, so each page is its own NavBackStackEntry and its own ViewModelStore; pushing a sub-page leaves the previous entry — and its SchemaViewModel — 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 the ProcessLifecycleOwner check at :1038 only 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 + a LocalLifecycleOwner-driven pause, or simply gating the probe on the page being resumed, matches what the ProcessLifecycleOwner check was already reaching for.

N3 — a deferred openPage fires 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, finishRun sets openPage (: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

  • B1 — the run lock is asymmetric. runAction:835 and submitForm:1262 both guard on runningAction only; a form submit sets submittingForm, so every action stays live for the whole request (minutes on a multipart upload) and the second runId response cancels the first follow.
  • B2 — a wedged legacy progressTopic run now freezes the forms too (runInFlight = state.runningAction != null, SchemaScreen.kt:282), and the escape hatch is gated on state.runId != null (:255), which is null for the legacy path.
  • B3TaskRun.id (Task.kt:28) is still the one required field; a status delta without it fails the whole TaskEvent decode and is dropped silently at :1094.
  • S4 — a refused refresh topic (:428) fires one refetchAll() 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 followRun with no test around it.

Review at `eded824`. `./gradlew testDebugUnitTest` green. `fix(schema): the probe's blind spots, and nav state out of the ViewModel` closes 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 - **S1** `.onFailure { if (it is CancellationException) throw it }` (`:983`) — and it sits before the failure accounting, so a cancelled probe no longer dirties the budget. - **S2** the `frames` snapshot (`: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. - **S3** `openPage` as one-shot state, acknowledged via `pageOpened()` *before* `onNavigateSub` (`SchemaScreen.kt:125-131`), and the `LaunchedEffect` is above the loading/error early-returns so it can't be composed away. Both details are the ones that usually get this pattern wrong. - **S5** the single loop with `SystemClock.elapsedRealtime()` — monotonic, right clock, no per-frame allocation. - **S6** `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.** `:985` returns `true` for the stale case, and the loop then unconditionally runs `baseline = elapsedRealtime()` / `delayMs = min(delayMs * 2, …)` (`:1045-1046`). Because `baseline` is now *ahead* of `lastFrameAtMs`, the `lastFrameAtMs > baseline` reset at `:1026` can'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"). `probeFailures` does get reset by the collector, so only the cadence is wrong. Distinguish stale from healthy-still-running in the return, or set `baseline = lastFrameAtMs; delayMs = FOLLOW_SILENCE_MS` on the stale path. **N2 — navigating away doesn't orphan the follow, it hides it — and the KDoc says otherwise.** `:935` claims "navigating to another page orphans the follow". `MainScaffold.kt:335` puts every module page on one `composable("module/{name}?path={path}")` route, so each page is its own `NavBackStackEntry` and its own `ViewModelStore`; pushing a sub-page leaves the previous entry — and its `SchemaViewModel` — 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 the `ProcessLifecycleOwner` check at `:1038` only 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` + a `LocalLifecycleOwner`-driven pause, or simply gating the probe on the page being resumed, matches what the `ProcessLifecycleOwner` check was already reaching for. **N3 — a deferred `openPage` fires 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, `finishRun` sets `openPage` (`: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 - **B1** — the run lock is asymmetric. `runAction:835` and `submitForm:1262` both guard on `runningAction` only; a form submit sets `submittingForm`, so every action stays live for the whole request (minutes on a multipart upload) and the second `runId` response cancels the first follow. - **B2** — a wedged legacy `progressTopic` run now freezes the forms too (`runInFlight = state.runningAction != null`, `SchemaScreen.kt:282`), and the escape hatch is gated on `state.runId != null` (`:255`), which is null for the legacy path. - **B3** — `TaskRun.id` (`Task.kt:28`) is still the one required field; a `status` delta without it fails the whole `TaskEvent` decode and is dropped silently at `:1094`. - **S4** — a refused refresh topic (`:428`) fires one `refetchAll()` 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 `followRun` with no test around it.
Closes the open findings of the last review round (B1-B3, S4, N1-N3).

- B1: the run lock is one derived flag, `runLocked`, tested by both write
  paths (runAction, submitForm) and mirrored by the screen (ActionsRow,
  FormCard). A form submit holds it from the moment it POSTs — its response
  decides whether it starts a run, so for the whole request it is a run
  waiting to be named. The confirm dialog funnels into the same guard.
- B2: "Stop following" now shows for any running action, runId or not — the
  legacy progressTopic path is the transport that most needs the exit and
  was the only one without it. endRun cancels the legacy stream too, and a
  runEpoch guard keeps a POST response that lands after "stop following"
  from resurrecting the lock the operator just escaped.
- B3: TaskRun.id is defaulted like every other field — a status delta has
  no reason to repeat the id already in the topic name, and as the last
  required field it silently killed exactly that frame. The one silent
  decode failure in the follow now logs.
- S4: a refused refresh topic is recorded (a set, not a count — the policy
  gate re-subscribes on mode changes) and logged; the interval fallback
  stops trusting a connected socket once every topic is refused.
- N1: the probe cadence lives in ProbeCadence, pure and tested. A frame
  landing mid-probe re-baselines instead of doubling the delay — the one
  case that proved the channel alive was the one that backed it off.
- N2: the screen reports its NavBackStackEntry lifecycle; the probe pauses
  while the page is parked on the back stack, where the process-level
  foreground check cannot see it. followRun's KDoc now tells the truth
  about navigation: the follow survives it, only the probe sleeps.
- N3: a deferred successPage is dropped when the run ends off-screen —
  navigation that fires the moment the operator comes back is a bounce
  they never asked for. Decision documented at the site.

./gradlew testDebugUnitTest green (83 tests, ProbeCadence and the id-less
status delta among them).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

ba5fe5f answers the open items from the review at eded824. ./gradlew testDebugUnitTest green (83).

B1 — the lock is one derived flag now: runLocked = runningAction != null || submittingForm != null, tested by runAction and submitForm in the model and mirrored by ActionsRow and FormCard on the screen. A form submit holds it from the moment it POSTs. The confirm dialog funnels into runAction'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), endRun cancels the legacy progressJob too, and a runEpoch counter (bumped by endRun and page turns, captured at press) keeps a POST response that lands after "stop following" from resurrecting the lock — it refetches instead of following.

B3TaskRun.id defaulted to 0, with the reasoning in its KDoc (a status delta has no reason to repeat the id already in the topic name). The TaskEvent decode failure now logs — it was the only silent one. Test added: an id-less {"type":"status"} delta decodes and reads terminal.

S4LiveEvent.Rejected on a refresh topic is recorded and logged instead of firing a refetch. Recorded in a set of topic names, not a count: RefreshPolicy.gate is flatMapLatest over 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 compares frames before/after the probe and skips probed() on the stale path, so the next waitFor re-baselines on the frame — base delay, no doubling. ProbeCadenceTest pins the regression case by name.

N2 — the screen reports its NavBackStackEntry lifecycle via setPageVisible (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 in finishRun.

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 (fake ApiClient/LiveClient + runTest) remains open — it needs both classes behind interfaces and is a refactor I'd rather not fold into a review-fix commit; ProbeCadence moves the probe's decision logic out of the coroutine the same way SubState did for the reconnect story.

`ba5fe5f` answers the open items from the review at `eded824`. `./gradlew testDebugUnitTest` green (83). **B1** — the lock is one derived flag now: `runLocked = runningAction != null || submittingForm != null`, tested by `runAction` and `submitForm` in the model and mirrored by `ActionsRow` and `FormCard` on the screen. A form submit holds it from the moment it POSTs. The confirm dialog funnels into `runAction`'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), `endRun` cancels the legacy `progressJob` too, and a `runEpoch` counter (bumped by `endRun` and 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.id` defaulted to `0`, with the reasoning in its KDoc (a status delta has no reason to repeat the id already in the topic name). The `TaskEvent` decode failure now logs — it was the only silent one. Test added: an id-less `{"type":"status"}` delta decodes and reads terminal. **S4** — `LiveEvent.Rejected` on a refresh topic is recorded and logged instead of firing a refetch. Recorded in a **set** of topic names, not a count: `RefreshPolicy.gate` is `flatMapLatest` over 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 compares `frames` before/after the probe and skips `probed()` on the stale path, so the next `waitFor` re-baselines on the frame — base delay, no doubling. `ProbeCadenceTest` pins the regression case by name. **N2** — the screen reports its `NavBackStackEntry` lifecycle via `setPageVisible` (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 in `finishRun`. 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 (fake `ApiClient`/`LiveClient` + `runTest`) remains open — it needs both classes behind interfaces and is a refactor I'd rather not fold into a review-fix commit; `ProbeCadence` moves the probe's decision logic out of the coroutine the same way `SubState` did for the reconnect story.
thisilike requested changes 2026-08-12 22:44:02 +02:00
Dismissed
thisilike left a comment

Review at ba5fe5f. ./gradlew testDebugUnitTest green.

B1, B2, B3, N1, N2, N3 are closed, and ProbeCadence + ProbeCadenceTest is 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. endRun cancelling progressJob closes 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. epoch is captured at press time and compared only in the runId branch. The other three write run state unconditionally:

if (code >= 400)            { _state.update { it.copy(runningAction = null, actionError = ) }; progressJob?.cancel() }
else if (runId != null)     { if (runEpoch != epoch)  else followRun(runId) }   // ← the only check
else if (progressTopic==null){ _state.update { it.copy(runningAction = null) }; refetchAll() }

onFailure = { _state.update { it.copy(runningAction = null, actionError = ) }; progressJob?.cancel() }

Repro — every step is possible only because this commit gated the exit on runningAction:

  1. Action A, slow route. runningAction = "A", epoch E.
  2. Operator taps Stop following (new in this commit — previously gated on runId, so unreachable for an action still POSTing). endRun releases the lock, epoch → E+1.
  3. Operator presses action B. Lock reacquired, runningAction = "B".
  4. A's POST finally answers 500 — or fails, or answers a plain 2xx.
  5. runningAction = null. B's lock is released while B is still in flight. B's spinner clears, B's actionError is overwritten by A's, B's progressJob is cancelled (it's a field now, and it belongs to B). A third writer can claim. If B's response later carries a runId with B's own epoch it will happily follow — after the screen has already said B was over.

submitForm captures no epoch at all. That path is unreachable today only because the exit isn't shown while submittingForm is 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. One if (runEpoch != epoch) return@fold (plus refetchAll) at the top of both fold arms, in both writers.

B5 — the lock and the exit are gated on different predicates

  • Lock: runningAction != null || submittingForm != null (SchemaViewModel:890, mirrored SchemaScreen.kt:149).
  • Exit: runId != null || runningAction != null (SchemaScreen.kt:289).

The gap is submittingForm != null with no runningAction: 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 writeTimeout override), 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 on runLocked and let stopFollowing abandon 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 — pageVisible conflates "another page is on top" with "the app isn't resumed this second". SchemaScreen.kt:135-146 derives it from isAtLeast(RESUMED). The notification shade, a permission dialog, a transient pause all drop the entry below RESUMED. finishRun:1312 then discards the form's successPage — 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. :495 computes pushCovered = refresh.topics.size > rejectedRefreshTopics.size || hasFollow, but hasFollow is 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 keeps pushCovered = true and never falls back to the interval — the exact failure just fixed one branch up.

S9 — "one lock" is defined twice. SchemaViewModel:890 and SchemaScreen.kt:149 are the same expression, in a commit titled one lock for every writer. Put it on SchemaUiState as a derived val runLocked and 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 are if (runLocked) return outside the coroutine; the write that closes the lock is inside viewModelScope.launch. It holds only because viewModelScope is Main.immediate and 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 before launch and the invariant stops being load-bearing on a dispatcher detail.

S11 — one staleness rule, two implementations. probe() snapshots frames and discards a stale response (:1061, :1072); the loop snapshots frames again to skip cadence.probed() (:1129, :1131). Consistent today only because nothing suspends between them. A tri-state return from probe() (ended / stale / silent) collapses it — and is what ProbeCadence'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_MS and 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. pageVisible flipping true is the natural reset point.

Nits

  • ProbeCadence has no Compose or Android dependency but lives in ui.schema. It's model logic; data/ or a follow/ package says what it is.
  • waitFor() reads as a query and mutates baseline/delayMs. Documented, but a name like tick() stops the next reader from calling it twice for a peek.
  • TaskRun.id = 0 makes "absent" and "zero" the same value. Nothing reads run.id today; a retry capability keyed on the linked run would.

Out of scope, but it's in these files

SchemaScreen.kt:167ErrorRetry(state.error!!, onRetry = { viewModel.start(module, subPath) }). start() returns early when started && 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 sealed RunState) holding label, epoch, origin, deferred page and follow job, with a single claim() / release() pair that every writer goes through and one derived locked the 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. ProbeCadence is proof the approach works here — it's the one piece of this machinery that stopped producing findings the moment it was extracted and pinned.

Review at `ba5fe5f`. `./gradlew testDebugUnitTest` green. B1, B2, B3, N1, N2, N3 are closed, and `ProbeCadence` + `ProbeCadenceTest` is 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. `endRun` cancelling `progressJob` closes 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`. `epoch` is captured at press time and compared only in the `runId` branch. The other three write run state unconditionally: ```kotlin if (code >= 400) { _state.update { it.copy(runningAction = null, actionError = …) }; progressJob?.cancel() } else if (runId != null) { if (runEpoch != epoch) … else followRun(runId) } // ← the only check else if (progressTopic==null){ _state.update { it.copy(runningAction = null) }; refetchAll() } … onFailure = { _state.update { it.copy(runningAction = null, actionError = …) }; progressJob?.cancel() } ``` Repro — every step is possible only because this commit gated the exit on `runningAction`: 1. Action A, slow route. `runningAction = "A"`, epoch `E`. 2. Operator taps **Stop following** (new in this commit — previously gated on `runId`, so unreachable for an action still POSTing). `endRun` releases the lock, epoch → `E+1`. 3. Operator presses action B. Lock reacquired, `runningAction = "B"`. 4. A's POST finally answers 500 — or fails, or answers a plain 2xx. 5. `runningAction = null`. **B's lock is released while B is still in flight.** B's spinner clears, B's `actionError` is overwritten by A's, B's `progressJob` is cancelled (it's a field now, and it belongs to B). A third writer can claim. If B's response later carries a `runId` with B's own epoch it will happily follow — after the screen has already said B was over. `submitForm` captures no epoch at all. That path is unreachable today only because the exit isn't shown while `submittingForm` is 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. One `if (runEpoch != epoch) return@fold` (plus `refetchAll`) at the top of both `fold` arms, in both writers. ### B5 — the lock and the exit are gated on different predicates - Lock: `runningAction != null || submittingForm != null` (`SchemaViewModel:890`, mirrored `SchemaScreen.kt:149`). - Exit: `runId != null || runningAction != null` (`SchemaScreen.kt:289`). The gap is `submittingForm != null` with no `runningAction`: **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 `writeTimeout` override), 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 on `runLocked` and let `stopFollowing` abandon 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 — `pageVisible` conflates "another page is on top" with "the app isn't resumed this second".** `SchemaScreen.kt:135-146` derives it from `isAtLeast(RESUMED)`. The notification shade, a permission dialog, a transient pause all drop the entry below RESUMED. `finishRun:1312` then **discards** the form's `successPage` — 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.** `:495` computes `pushCovered = refresh.topics.size > rejectedRefreshTopics.size || hasFollow`, but `hasFollow` is 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 keeps `pushCovered = true` and never falls back to the interval — the exact failure just fixed one branch up. **S9 — "one lock" is defined twice.** `SchemaViewModel:890` and `SchemaScreen.kt:149` are the same expression, in a commit titled *one lock for every writer*. Put it on `SchemaUiState` as a derived `val runLocked` and 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 are `if (runLocked) return` *outside* the coroutine; the write that closes the lock is *inside* `viewModelScope.launch`. It holds only because `viewModelScope` is `Main.immediate` and 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 before `launch` and the invariant stops being load-bearing on a dispatcher detail. **S11 — one staleness rule, two implementations.** `probe()` snapshots `frames` and discards a stale response (`:1061`, `:1072`); the loop snapshots `frames` again to skip `cadence.probed()` (`:1129`, `:1131`). Consistent today only because nothing suspends between them. A tri-state return from `probe()` (ended / stale / silent) collapses it — and is what `ProbeCadence`'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_MS` and 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. `pageVisible` flipping true is the natural reset point. ## Nits - `ProbeCadence` has no Compose or Android dependency but lives in `ui.schema`. It's model logic; `data/` or a `follow/` package says what it is. - `waitFor()` reads as a query and mutates `baseline`/`delayMs`. Documented, but a name like `tick()` stops the next reader from calling it twice for a peek. - `TaskRun.id = 0` makes "absent" and "zero" the same value. Nothing reads `run.id` today; a `retry` capability 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 when `started && 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 `var`s (`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 sealed `RunState`) holding label, epoch, origin, deferred page and follow job, with a single `claim()` / `release()` pair that every writer goes through and one derived `locked` the 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. `ProbeCadence` is 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 of PR #4 review:

- B4: runEpoch checked at the TOP of both fold arms in runAction and
  submitForm - a response whose lock was released mid-flight touches no
  run state at all (not even progressJob, which a new claimant may own),
  it refetches and walks away. The branch no longer decides; the epoch does.
- B5: endRun releases the submit half of the lock too, and the escape
  hatch is gated on the same runLocked the buttons disable on - an
  in-flight form submit now shows "Stop waiting" instead of locking the
  page with no way out.
- S9: runLocked is derived once, on SchemaUiState; the model's guards and
  the screen read the same fact.
- S10: the lock is claimed synchronously with the press, before the
  coroutine - the mutual exclusion no longer hangs on Main.immediate.
- S7: page visibility is STARTED, not RESUMED - a notification shade or
  permission dialog no longer discards a form's successPage.
- S8: a rejected log follow is recorded and stops counting as push
  coverage, so a page whose only push was that follow falls back to its
  interval.
- S11: probe() returns ENDED/STALE/SILENT; the staleness rule lives in
  one place and the loop acts on the verdict.
- S12: the probe loop wakes on the page coming back on screen and resets
  the cadence - no more up-to-30s of stale run state after returning.
- ProbeCadence moved to data/follow, waitFor renamed tick (it mutates),
  reset() added and tested.
- The error screen's retry button retries again (start() no-ops on the
  same page; retry() clears `started` first).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Round 7 addressed in 2771957. ./gradlew testDebugUnitTest and assembleDebug green.

Blocking

B4 — the epoch, not the branch, decides ownership. Both fold arms in runAction and submitForm now 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, not progressJob (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. endRun releases the submit half too (submittingForm / uploadPercent / uploading), and the escape-hatch row is gated on state.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

  • S7 — page visibility is STARTED, not RESUMED. A shade pull or permission dialog only pauses the entry; back-stacked entries fall below STARTED. finishRun no longer discards a successPage over a notification.
  • S8rejectedFollows (block indexes): Rejected records + logs, any delivered event re-arms, and pushCovered counts only follows that weren't refused. Both halves of the S4 fix now exist.
  • S9runLocked is derived once, on SchemaUiState. Model guards, screen disabled-states and the exit all read it; there is no second definition site left to miss.
  • S10 — guard-then-claim happens synchronously with the press, before the launch. The mutual exclusion no longer rides on Main.immediate; the clientOrNull == null path releases the claim it just took.
  • S11probe() returns ENDED / STALE / SILENT. The staleness rule lives in probe() alone; the loop acts on the verdict and no longer re-derives it from frames.
  • S12pageOnScreen is a StateFlow; the loop sleeps in withTimeoutOrNull(wait) { pageOnScreen.dropWhile { it }.first { it } }, so it wakes on the off-screen→on-screen edge whichever state the sleep started in, and cadence.reset(lastFrameAtMs) forgets the backoff. A dead channel is probed immediately on return; a recent frame keeps its quiet window. ProbeCadenceTest pins both halves.

Nits

  • ProbeCadence moved to data/follow/ — no Compose or Android dependency, and the package now says what it is.
  • waitFortick, with the mutation named in its KDoc.
  • TaskRun.id = 0 left as-is per your note — nothing reads it today; the day a retry capability keys on it, it becomes nullable.

Out of scope, taken anyway

The dead retry button: retry() clears started before re-entering start(), and the error screen calls it instead of start() directly.

On the pattern

Not the full RunClaim extraction 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

Round 7 addressed in `2771957`. `./gradlew testDebugUnitTest` and `assembleDebug` green. ## Blocking **B4 — the epoch, not the branch, decides ownership.** Both `fold` arms in `runAction` *and* `submitForm` now 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, not `progressJob` (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.** `endRun` releases the submit half too (`submittingForm` / `uploadPercent` / `uploading`), and the escape-hatch row is gated on `state.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 - **S7** — page visibility is `STARTED`, not `RESUMED`. A shade pull or permission dialog only pauses the entry; back-stacked entries fall below `STARTED`. `finishRun` no longer discards a successPage over a notification. - **S8** — `rejectedFollows` (block indexes): `Rejected` records + logs, any delivered event re-arms, and `pushCovered` counts only follows that weren't refused. Both halves of the S4 fix now exist. - **S9** — `runLocked` is derived once, on `SchemaUiState`. Model guards, screen disabled-states and the exit all read it; there is no second definition site left to miss. - **S10** — guard-then-claim happens synchronously with the press, before the `launch`. The mutual exclusion no longer rides on `Main.immediate`; the `clientOrNull == null` path releases the claim it just took. - **S11** — `probe()` returns `ENDED / STALE / SILENT`. The staleness rule lives in `probe()` alone; the loop acts on the verdict and no longer re-derives it from `frames`. - **S12** — `pageOnScreen` is a `StateFlow`; the loop sleeps in `withTimeoutOrNull(wait) { pageOnScreen.dropWhile { it }.first { it } }`, so it wakes on the off-screen→on-screen edge whichever state the sleep started in, and `cadence.reset(lastFrameAtMs)` forgets the backoff. A dead channel is probed immediately on return; a recent frame keeps its quiet window. `ProbeCadenceTest` pins both halves. ## Nits - `ProbeCadence` moved to `data/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 = 0` left as-is per your note — nothing reads it today; the day a `retry` capability keys on it, it becomes nullable. ## Out of scope, taken anyway The dead retry button: `retry()` clears `started` before re-entering `start()`, and the error screen calls it instead of `start()` directly. ## On the pattern Not the full `RunClaim` extraction 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](https://claude.com/claude-code)
thisilike requested changes 2026-08-12 23:05:47 +02:00
Dismissed
thisilike left a comment

Review at 2771957. ./gradlew testDebugUnitTest green.

B5, S7–S12 and the dead retry button are all properly closed, and several of them better than I suggested: ProbeResult collapses the duplicated staleness rule into one verdict the loop merely acts on; runLocked on SchemaUiState makes the lock genuinely one fact; the dropWhile { 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:

client.modulePostMultipart(module, path, fields, files) { sent, total ->
    _state.update { it.copy(uploadPercent = pct?.coerceIn(0, 100)) }
}

stopFollowing() releases the lock but explicitly does not cancel the request — so an abandoned upload keeps streaming and keeps writing uploadPercent. Concretely:

  1. Form A, large file. submittingForm = "A", upload running.
  2. Stop waiting (new this commit). endRun clears submittingForm/uploading/uploadPercent, bumps the epoch. A's upload carries on.
  3. Form B submitted. submittingForm = "B".
  4. A's callback fires. FormCard.kt:337 renders submitting == block.submit.label && uploadPercent != nullform 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. Captures runId at tap time, writes actionError / runStatus / runCancellable at response time, checks nothing in between. Tap Cancel → tap Stop following → press another action → the refusal lands and overwrites the new claimant's banner, or runStatus = CANCELLING is written onto run state that no longer belongs to the run that was cancelled.

(c) Releases don't bump the epoch. Only start:365 and endRun:1428 bump. 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 inside release() 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 next tick returns <= 0 and 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_MS of the last one) so a fidgety back button can't drive the request rate.

S14 — withTimeoutOrNull { … } != null on a block that returns Boolean. :1189-1193. It works because first { it } can only yield true, so false is unreachable — but the expression compares a Boolean? against null, where "returned false" and "timed out" would be indistinguishable the moment that changes. == true is the same length and can't rot.

S15 — the successPage discard is still an instantaneous test. finishRun:1390 reads pageOnScreen.value at 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.ProbeCadence sits between data.model.* and data.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.
  • ProbeCadence moving to data/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. ProbeCadenceTest grew again and is genuinely good — the coming back on screen forgets the backoff test 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.

Review at `2771957`. `./gradlew testDebugUnitTest` green. B5, S7–S12 and the dead retry button are all properly closed, and several of them better than I suggested: `ProbeResult` collapses the duplicated staleness rule into one verdict the loop merely acts on; `runLocked` on `SchemaUiState` makes the lock genuinely one fact; the `dropWhile { 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`: ```kotlin client.modulePostMultipart(module, path, fields, files) { sent, total -> _state.update { it.copy(uploadPercent = pct?.coerceIn(0, 100)) } } ``` `stopFollowing()` releases the lock but explicitly does not cancel the request — so an abandoned upload keeps streaming and keeps writing `uploadPercent`. Concretely: 1. Form A, large file. `submittingForm = "A"`, upload running. 2. **Stop waiting** (new this commit). `endRun` clears `submittingForm`/`uploading`/`uploadPercent`, bumps the epoch. A's upload carries on. 3. Form B submitted. `submittingForm = "B"`. 4. A's callback fires. `FormCard.kt:337` renders `submitting == 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`.** Captures `runId` at tap time, writes `actionError` / `runStatus` / `runCancellable` at response time, checks nothing in between. Tap Cancel → tap Stop following → press another action → the refusal lands and overwrites the new claimant's banner, or `runStatus = CANCELLING` is written onto run state that no longer belongs to the run that was cancelled. **(c) Releases don't bump the epoch.** Only `start:365` and `endRun:1428` bump. 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 inside `release()` 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 next `tick` returns `<= 0` and 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_MS` of the last one) so a fidgety back button can't drive the request rate. **S14 — `withTimeoutOrNull { … } != null` on a block that returns `Boolean`.** `:1189-1193`. It works because `first { it }` can only yield `true`, so `false` is unreachable — but the expression compares a `Boolean?` against null, where "returned false" and "timed out" would be indistinguishable the moment that changes. `== true` is the same length and can't rot. **S15 — the successPage discard is still an instantaneous test.** `finishRun:1390` reads `pageOnScreen.value` at 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.ProbeCadence` sits between `data.model.*` and `data.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. - `ProbeCadence` moving to `data/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. `ProbeCadenceTest` grew again and is genuinely good — the `coming back on screen forgets the backoff` test 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 of PR #4 review:

- B6: run-state ownership is one fact with one gate. New RunClaim
  (data/follow) holds the epoch, the claim's origin and its deferred
  successPage; claim() hands the writer a token, release() retires every
  token unconditionally - the bump lives inside release, so a completion
  path cannot open the lock and forget it. releaseRun() in the ViewModel
  is now the ONLY place the lock opens; every completion path (400,
  failure, plain 2xx, client-null, the legacy stream's done line) goes
  through it and therefore bumps. The writers that had no check now
  present a token: the multipart progress callback (epoch is @Volatile -
  the callback outlives an abandoned claim by design, on an IO thread),
  and cancelRun, which captures the token at tap time so a late refusal
  cannot overwrite the next claimant's banner or write CANCELLING onto a
  run nobody asked to stop. The submit-to-follow handover swaps
  submittingForm for runningAction in one write - no frame where the
  lock is open. RunClaimTest pins the token lifecycle, including the
  release-between-two-claims collision.
- S13: ProbeCadence gains a floor - no probe fires within silenceMs of
  the previous one, however often reset() re-arms the schedule. Paging
  away and back is user-driven; without the floor every return to a page
  with a dead channel probed at once. Tested, including the toggle case.
- S14: the page-return edge detector compares == true, not != null -
  "returned false" and "timed out" stay distinguishable if the block
  ever can yield false.
- S15: "the operator is still here" is a duration, not the instant the
  terminal frame lands - a form-run's successPage now survives a shade
  pull or a short app switch (5s grace since the page left the screen)
  instead of being discarded for good.
- stopFollowing renamed abandonRun: it ends three different waits (a
  followed run, a legacy stream, an in-flight POST/upload) and the
  button already switched labels; the name follows.
- data.follow imports sorted where they belong.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Round 8 pushed as 1ca7b52. ./gradlew assembleDebug and ./gradlew testDebugUnitTest green (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 RunClaim in data/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's done line — that last one closes (c): a POST response landing after the stream already declared the outcome now finds its token dead and just refetches.
  • (a) the multipart progress callback presents its token before writing uploadPercent. The epoch is @Volatile for 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.
  • (b) cancelRun captures claim.token at tap time and checks it when the verdict lands — a refusal arriving after stop-following + a new claim writes nothing.
  • The submit→follow handover swaps submittingForm for runningAction in one _state.update — no frame where neither is set and the lock reads open.

RunClaimTest pins the lifecycle, including the collision that was (c): claim A, release, claim B — A's token must not own B's state.

Should-fix

  • S13: ProbeCadence gained the floor — tick() is never due within silenceMs of the last probe, however often reset() 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 existing coming back on screen test now asserts "due one base window after the last probe" instead of "due immediately".
  • S14: == true.
  • S15: "the operator is still here" is now a duration — leftScreenAtMs is stamped on the visible→invisible edge and finishRun grants 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

  • stopFollowingabandonRun, with the three waits it ends named in the KDoc.
  • data.follow imports sorted where they belong.

Standing

The pure surface grew again instead of the coroutine getting a fixture: RunClaim joins ProbeCadence and SubState as 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 versionName bump — 0.31.0 is still unreleased on this branch.

Round 8 pushed as `1ca7b52`. `./gradlew assembleDebug` and `./gradlew testDebugUnitTest` green (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 `RunClaim` in `data/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's `done` line — that last one closes (c): a POST response landing after the stream already declared the outcome now finds its token dead and just refetches. - **(a)** the multipart progress callback presents its token before writing `uploadPercent`. The epoch is `@Volatile` for 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. - **(b)** `cancelRun` captures `claim.token` at tap time and checks it when the verdict lands — a refusal arriving after stop-following + a new claim writes nothing. - The submit→follow handover swaps `submittingForm` for `runningAction` in **one** `_state.update` — no frame where neither is set and the lock reads open. `RunClaimTest` pins the lifecycle, including the collision that was (c): claim A, release, claim B — A's token must not own B's state. ## Should-fix - **S13**: `ProbeCadence` gained the floor — `tick()` is never due within `silenceMs` of the last probe, however often `reset()` 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 existing `coming back on screen` test now asserts "due one base window after the last probe" instead of "due immediately". - **S14**: `== true`. - **S15**: "the operator is still here" is now a duration — `leftScreenAtMs` is stamped on the visible→invisible edge and `finishRun` grants 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.follow` imports sorted where they belong. ## Standing The pure surface grew again instead of the coroutine getting a fixture: `RunClaim` joins `ProbeCadence` and `SubState` as 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 `versionName` bump — 0.31.0 is still unreleased on this branch.
thisilike requested changes 2026-08-13 08:00:35 +02:00
Dismissed
thisilike left a comment

Review at 1ca7b52. ./gradlew testDebugUnitTest green.

B6(a), B6(b), B6(c), S13, S14 and both nits are closed, and RunClaim with the bump inside release() 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:1404 decides whether to set openPage from the page's visibility at the moment the terminal frame lands. openPage is consumed by LaunchedEffect(openPage) when the screen composes. A NavBackStackEntry below the top is not composed, so the flag sits in state until the operator navigates back.

  1. Form starts a run, operator pushes a sub-page. leftScreenAtMs stamped.
  2. Run succeeds 2s later — inside SUCCESS_NAV_GRACE_MS, so openPage is set.
  3. Operator works elsewhere for ten minutes.
  4. They navigate back to the page. LaunchedEffect fires 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-1624 sets openPage with 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 openPage when it is set, and have the effect (or pageOpened) discard one older than the grace.

B8 — releasing on the legacy stream's "done" retires the token of the POST that started it

:1004. The progressTopic stream 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 fails claim.owns(token) at :1024, hits the disowned branch, and returns after refetchAll().

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 runningAction without 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 — releaseRun is documented as the only gate and doesn't check the token. Six call sites: :977 and :1519 trivially own (immediately after claiming), :1033/:1045/:1056 and :1580/:1621/:1632 sit inside an owns check, :1438 is the deliberate unconditional abandon — and :1004 is 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 — RunClaim cannot distinguish "held" from "free", so it is not yet the mutual exclusion. claim() sets the payload and returns the current epoch without bumping, so two claim() 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 in SchemaUiState.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 = … } then claim.claim()), correct only because nothing suspends between them — the same reasoning S10 was raised about. A held flag, claim() rejecting when held, and locked derived from the claim would put all of it in one object.

S18 — probed() records probes that never ran. :1222 is deliberately outside the foreground check so a skipped iteration still advances the baseline — but it now also stamps probedAtMs, 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 to silenceMs beyond what the reset intended. Split it: a skipped() that moves baseline/delayMs and leaves probedAtMs alone.

S19 — @Volatile covers epoch and nothing else. The class doc says the token is read from the upload callback's IO thread; fromForm and successPage are plain fields on the same object. Correct today because IO only calls owns() — but nothing in the type says so, and the next IO-side reader will find successPage sitting right there. Either swap an immutable claim through an AtomicReference, or make the cross-thread surface a single accessor and keep the rest private.

Nits

  • RunClaimTest covers 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 in claim().
  • Import order fixed; still no ktlint/detekt/spotless gate on ~2000 lines.
  • abandonRun rename is clean — no stale stopFollowing references anywhere in app/src.

Standing

ProbeCadence and RunClaim are 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 in runAction / submitForm / finishRun / endRun, and those four functions have never had a test.

Review at `1ca7b52`. `./gradlew testDebugUnitTest` green. B6(a), B6(b), B6(c), S13, S14 and both nits are closed, and `RunClaim` with the bump inside `release()` 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:1404` decides whether to set `openPage` from the page's visibility **at the moment the terminal frame lands**. `openPage` is consumed by `LaunchedEffect(openPage)` **when the screen composes**. A NavBackStackEntry below the top is not composed, so the flag sits in state until the operator navigates back. 1. Form starts a run, operator pushes a sub-page. `leftScreenAtMs` stamped. 2. Run succeeds 2s later — inside `SUCCESS_NAV_GRACE_MS`, so `openPage` is set. 3. Operator works elsewhere for ten minutes. 4. They navigate back to the page. `LaunchedEffect` fires 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-1624` sets `openPage` with **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 `openPage` when it is set, and have the effect (or `pageOpened`) discard one older than the grace. ### B8 — releasing on the legacy stream's "done" retires the token of the POST that started it `:1004`. The `progressTopic` stream 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 fails `claim.owns(token)` at `:1024`, hits the disowned branch, and returns after `refetchAll()`. 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 `runningAction` without 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 — `releaseRun` is documented as the only gate and doesn't check the token.** Six call sites: `:977` and `:1519` trivially own (immediately after claiming), `:1033`/`:1045`/`:1056` and `:1580`/`:1621`/`:1632` sit inside an `owns` check, `:1438` is the deliberate unconditional abandon — and `:1004` is 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 — `RunClaim` cannot distinguish "held" from "free", so it is not yet the mutual exclusion.** `claim()` sets the payload and returns the *current* epoch without bumping, so two `claim()` 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 in `SchemaUiState.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 = … }` then `claim.claim()`), correct only because nothing suspends between them — the same reasoning S10 was raised about. A `held` flag, `claim()` rejecting when held, and `locked` derived from the claim would put all of it in one object. **S18 — `probed()` records probes that never ran.** `:1222` is deliberately outside the foreground check so a skipped iteration still advances the baseline — but it now also stamps `probedAtMs`, 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 to `silenceMs` beyond what the reset intended. Split it: a `skipped()` that moves `baseline`/`delayMs` and leaves `probedAtMs` alone. **S19 — `@Volatile` covers `epoch` and nothing else.** The class doc says the token is read from the upload callback's IO thread; `fromForm` and `successPage` are plain fields on the same object. Correct today because IO only calls `owns()` — but nothing in the type says so, and the next IO-side reader will find `successPage` sitting right there. Either swap an immutable claim through an `AtomicReference`, or make the cross-thread surface a single accessor and keep the rest private. ## Nits - `RunClaimTest` covers 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 in `claim()`. - Import order fixed; still no ktlint/detekt/spotless gate on ~2000 lines. - `abandonRun` rename is clean — no stale `stopFollowing` references anywhere in `app/src`. ## Standing `ProbeCadence` and `RunClaim` are 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 in `runAction` / `submitForm` / `finishRun` / `endRun`, and those four functions have never had a test.
B7: `openPage` was tested for freshness where it is SET, and consumed by a
LaunchedEffect when the screen composes — which for an entry below the top of
the back stack is whenever the operator navigates back. A success landing
off-screen inside the grace parked a bounce that fired minutes later, verbatim
the outcome the KDoc argues against. The non-run submit path had no test at
all, and it is the slower one: a multipart upload answers after minutes.
Stamp the request in `requestPage`, and let `pageOpened` — the only place that
can know how long it waited — decide whether it is still wanted.

B8: the legacy stream's "done" released the claim, and the epoch bump retired
the token of the POST that started it. Its response then took the disowned
branch, so a 4xx was reported to nobody. The stream and the response are
independent transports with no ordering between them, so the action now ends
when BOTH have landed; `LegacyEnding` is that rule, testable in either order.

S16: `releaseRun(token, error)` no-ops when the token no longer owns, so the
documented gate is one the code actually passes through.

S17: `claim()` refuses while the state is held, which makes the claim the
mutual exclusion rather than an epoch counter beside one. No caller checks a
flag before starting work; `SchemaUiState.runLocked` is now only what the
screen disables on.

S18: `skipped()` splits off `probed()`. An off-screen tick moves the schedule
but not `probedAtMs`, so the floor no longer delays the first real probe after
the operator returns.

S19: the whole claim swaps through one AtomicReference. The upload callback
reads it from an IO thread, and visibility should not depend on which field
somebody remembered to mark @Volatile.

Tests: 103 green. New coverage for the claim's refusal and for owning nothing
when nobody holds, both legacy orderings, the probe floor across a skipped
tick, and the nav grace at consumption.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Owner

Pushed 3498205. ./gradlew testDebugUnitTest green — 103 tests.

B7 — the grace moved to the consuming end. requestPage stamps when the navigation was asked for; pageOpened returns the page only if the stamp is still inside SUCCESS_NAV_GRACE_MS, and spends the request either way. The screen's LaunchedEffect navigates to what pageOpened hands 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. finishRun no longer tests visibility at all, which also fixes the other half: the non-run submit at the end of submitForm goes through the same requestPage, so a multipart upload that answers after minutes is bounded by the same rule. leftScreenAtMs is gone — it had no other reader.

B8LegacyEnding. 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.

S16releaseRun(token, error) no-ops when the token no longer owns; the unconditional overload is now only endRun's deliberate abandon. Every call site that sits under a claim went through it, including the two that trivially own.

S17claim() returns null while the state is held, and both writers do claim.claim(...) ?: return before writing the state it guards. The _state.value.runLocked guards are gone: the exclusion is one act in one object, not a flag checked next to a claim taken. SchemaUiState.runLocked stays 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, and owns is false when nobody holds.

S18skipped() split out of probed(). It moves baseline and delayMs; only a probe that ran stamps probedAtMs. Regression test: probe at 10s, skipped tick at 30s, return at 32s — due at once, where stamping the skip held it off another 8s.

S19RunClaim holds one immutable Held in an AtomicReference, so fromForm and successPage are 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

Pushed `3498205`. `./gradlew testDebugUnitTest` green — 103 tests. **B7** — the grace moved to the consuming end. `requestPage` stamps when the navigation was asked for; `pageOpened` returns the page only if the stamp is still inside `SUCCESS_NAV_GRACE_MS`, and spends the request either way. The screen's `LaunchedEffect` navigates to what `pageOpened` hands 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. `finishRun` no longer tests visibility at all, which also fixes the other half: the non-run submit at the end of `submitForm` goes through the same `requestPage`, so a multipart upload that answers after minutes is bounded by the same rule. `leftScreenAtMs` is 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 only `endRun`'s deliberate abandon. Every call site that sits under a claim went through it, including the two that trivially own. **S17** — `claim()` returns `null` while the state is held, and both writers do `claim.claim(...) ?: return` **before** writing the state it guards. The `_state.value.runLocked` guards are gone: the exclusion is one act in one object, not a flag checked next to a claim taken. `SchemaUiState.runLocked` stays 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, and `owns` is false when nobody holds. **S18** — `skipped()` split out of `probed()`. It moves `baseline` and `delayMs`; only a probe that ran stamps `probedAtMs`. 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** — `RunClaim` holds one immutable `Held` in an `AtomicReference`, so `fromForm` and `successPage` are 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](https://claude.com/claude-code)
thisilike left a comment

Review at 3498205. ./gradlew testDebugUnitTest green.

B7, B8, S16, S17, S18, S19 and the RunClaimTest gap 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. requestPage stamps, pageOpened() decides and spends either way, screen does pageOpened()?.let { onNavigateSub(it) }. The mechanism is right for the case it was raised about: a NavBackStackEntry under another page leaves composition, so the LaunchedEffect is cancelled and re-runs on return — where the request is found stale and spent without navigating. And requestPage now 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 reaches releaseRun(token, msg) and reports, 2xx takes the new streamEnded branch and releases without a second refetch. LegacyEnding is 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() returning Int? makes the guard and the claim one act, and owns() requiring held closes the "token read out of a free claim" hole that nobody holding is nobody owning now pins.

Should fix

S21 — RunClaim presents itself as the mutual exclusion, uses an AtomicReference, and implements read-modify-write non-atomically.

fun claim(...): Int? {
    val current = state.get()
    if (current.held) return null
    state.set(current.copy(held = true, ...))   // get-then-set, not CAS
    return current.epoch
}
fun release() { state.set(Held(epoch = state.get().epoch + 1)) }

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 concurrent release() calls lose an epoch. compareAndSet in a small loop (or updateAndGet) costs nothing here and makes the type mean what it says. RunClaimTest is 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.

pageOnScreen no 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 a LaunchedEffect firing 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 explicit pageOnScreen check inside pageOpened(), makes the choice visible instead of inherited. SuccessNavTest covers navStillWanted well; 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. onFailure calls ending.responded(), ignores the answer, and writes actionError = 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. When responded() returns true, the ending is known; the transport error belongs in the feed, not in the verdict banner.

S23 — navStillWanted is the only extracted rule that did not move. It is an internal fun sitting in SchemaViewModel.kt while ProbeCadence, RunClaim and LegacyEnding all live in data/follow/. Same treatment, same place.

Nits

  • A refused claim() is a silent return. 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.
  • LegacyEnding is allocated for every action, including those with no progressTopic.
  • Downloads are still the one wedge with no exit: openAction holds downloading for 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 split downloading out, so it is the last asymmetry left from that split.
  • Still no ktlint/detekt/spotless gate.

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, finishRun and endRun still 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.

Review at `3498205`. `./gradlew testDebugUnitTest` green. B7, B8, S16, S17, S18, S19 and the `RunClaimTest` gap 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.** `requestPage` stamps, `pageOpened()` decides and spends either way, screen does `pageOpened()?.let { onNavigateSub(it) }`. The mechanism is right for the case it was raised about: a NavBackStackEntry under another page leaves composition, so the `LaunchedEffect` is cancelled and re-runs on return — where the request is found stale and spent without navigating. And `requestPage` now 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 reaches `releaseRun(token, msg)` and **reports**, 2xx takes the new `streamEnded` branch and releases without a second refetch. `LegacyEnding` is 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()` returning `Int?` makes the guard and the claim one act, and `owns()` requiring `held` closes the "token read out of a free claim" hole that `nobody holding is nobody owning` now pins. ## Should fix **S21 — `RunClaim` presents itself as the mutual exclusion, uses an `AtomicReference`, and implements read-modify-write non-atomically.** ```kotlin fun claim(...): Int? { val current = state.get() if (current.held) return null state.set(current.copy(held = true, ...)) // get-then-set, not CAS return current.epoch } fun release() { state.set(Held(epoch = state.get().epoch + 1)) } ``` 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 concurrent `release()` calls lose an epoch. `compareAndSet` in a small loop (or `updateAndGet`) costs nothing here and makes the type mean what it says. `RunClaimTest` is 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.** `pageOnScreen` no 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 a `LaunchedEffect` firing 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 explicit `pageOnScreen` check inside `pageOpened()`, makes the choice visible instead of inherited. `SuccessNavTest` covers `navStillWanted` well; 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.** `onFailure` calls `ending.responded()`, ignores the answer, and writes `actionError = 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. When `responded()` returns true, the ending is known; the transport error belongs in the feed, not in the verdict banner. **S23 — `navStillWanted` is the only extracted rule that did not move.** It is an `internal fun` sitting in `SchemaViewModel.kt` while `ProbeCadence`, `RunClaim` and `LegacyEnding` all live in `data/follow/`. Same treatment, same place. ## Nits - A refused `claim()` is a silent `return`. 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. - `LegacyEnding` is allocated for every action, including those with no `progressTopic`. - Downloads are still the one wedge with no exit: `openAction` holds `downloading` for 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 split `downloading` out, so it is the last asymmetry left from that split. - Still no ktlint/detekt/spotless gate. ## 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`, `finishRun` and `endRun` still 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.
thisilike requested changes 2026-08-13 14:15:51 +02:00
Dismissed
thisilike left a comment

Re-filing my review of 3498205 as 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

  1. S21 — RunClaim: make the read-modify-write atomic. claim() and release() are get-then-set on an AtomicReference. Main-only makes it correct; the class doc ("[claim] is the mutual exclusion itself") plus the atomic container invites the opposite assumption. compareAndSet in a small loop, or updateAndGet. 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.

  2. S20 — decide when the successPage request is consumed. pageOnScreen no longer takes part in the navigation decision at all; freshness-at-consumption is the whole rule, and consumption is a LaunchedEffect. 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 a pageOnScreen check inside pageOpened() — either is fine, but pick one so the behavior is stated rather than inherited.

  3. S22 — do not report a failure the app does not have. onFailure calls ending.responded(), discards the answer, and writes actionError = e.message. A POST that times out after the stream already said "done" puts "timeout" over work that visibly completed and was already refetched. When responded() returns true the ending is known; the transport error belongs in the feed, not the verdict banner.

  4. S23 — move navStillWanted to data/follow/. It is the only extracted rule still living in SchemaViewModel.kt, alongside three that were moved for exactly this reason.

  5. Nits, take or leave: a refused claim() is a silent return — reachable via a confirm dialog opened before something else took the state, where Confirm now closes and does nothing with no explanation; LegacyEnding is allocated for actions with no progressTopic; downloads remain the one wedge with no exit (openAction holds downloading for the whole transfer, only the read timeout ends it) — out of scope for the run lock, but this PR is what split downloading out; 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 LegacyEnding orderings and traced the composition lifecycle behind pageOpened(), rather than taking the commit message for either — and releaseRun(token, …), claim(): Int? and owns() requiring held are the right shapes for S16/S17/S19.

Post these and I will approve.

Re-filing my review of `3498205` as 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** 1. **S21 — `RunClaim`: make the read-modify-write atomic.** `claim()` and `release()` are get-then-set on an `AtomicReference`. Main-only makes it correct; the class doc ("[claim] is the mutual exclusion itself") plus the atomic container invites the opposite assumption. `compareAndSet` in a small loop, or `updateAndGet`. 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. 2. **S20 — decide when the successPage request is consumed.** `pageOnScreen` no longer takes part in the navigation decision at all; freshness-at-consumption is the whole rule, and consumption is a `LaunchedEffect`. 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 a `pageOnScreen` check inside `pageOpened()` — either is fine, but pick one so the behavior is stated rather than inherited. 3. **S22 — do not report a failure the app does not have.** `onFailure` calls `ending.responded()`, discards the answer, and writes `actionError = e.message`. A POST that times out after the stream already said "done" puts "timeout" over work that visibly completed and was already refetched. When `responded()` returns true the ending is known; the transport error belongs in the feed, not the verdict banner. 4. **S23 — move `navStillWanted` to `data/follow/`.** It is the only extracted rule still living in `SchemaViewModel.kt`, alongside three that were moved for exactly this reason. 5. **Nits, take or leave:** a refused `claim()` is a silent `return` — reachable via a confirm dialog opened before something else took the state, where Confirm now closes and does nothing with no explanation; `LegacyEnding` is allocated for actions with no `progressTopic`; downloads remain the one wedge with no exit (`openAction` holds `downloading` for the whole transfer, only the read timeout ends it) — out of scope for the run lock, but this PR is what split `downloading` out; 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 `LegacyEnding` orderings and traced the composition lifecycle behind `pageOpened()`, rather than taking the commit message for either — and `releaseRun(token, …)`, `claim(): Int?` and `owns()` requiring `held` are the right shapes for S16/S17/S19. Post these and I will approve.
The last cleanup pass on the review checklist.

RunClaim said "[claim] is the mutual exclusion itself" and then implemented
its read-modify-write as get-then-set on an AtomicReference. Main-only
callers made that correct, but the type's own doc plus an atomic container
invite the opposite assumption, and this is the primitive four rounds of
findings converged on: it should be the one place a reader does not have to
check the callers to know it is safe. claim() is now a CAS loop, release()
an updateAndGet, and RunClaimTest races both — not a proof of thread-safety,
but the shape get-then-set loses.

The successPage request now decides when it is consumed instead of
inheriting it. Consumption is a LaunchedEffect, and for a backgrounded app
the composition is retained, so whether the effect fired promptly or waited
for the window was the recomposer's business: prompt meant navigating to a
page the operator never asked for, late meant the grace expired. Gated on
repeatOnLifecycle(STARTED), so both ways of being gone land on the same
rule — the duration measured is a page that was on screen.

A transport failure after the legacy stream already said "done" no longer
overwrites the verdict. The work finished and the operator watched it
finish; a POST that times out afterwards is news about the request, so it
goes in the feed. Reporting an outcome the app does not have is what the
task-run change exists to stop, and this was the same lie with the sign
flipped.

navStillWanted moves to data/follow/ beside ProbeCadence, RunClaim and
LegacyEnding — it was the only extracted rule still living in the ViewModel.

Nits:
- a refused claim is no longer silent. It is reachable through a confirm
  dialog opened just before something else took the state, where Confirm
  closed the dialog and nothing happened at all.
- LegacyEnding is allocated only for an action that has a legacy stream,
  and its nullness IS that fact, so the two cannot drift.
- downloads get the exit every other wedge has. "Stop download" releases
  the screen, not the socket — the read is blocking, so it runs to its own
  end; a transfer sequence keeps an abandoned one's progress off the next
  download's line, and the job now guards a double press the label could
  not.

./gradlew assembleDebug and testDebugUnitTest green (105 tests), lintDebug
clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Owner

Checklist posted at db9ee59. ./gradlew assembleDebug, testDebugUnitTest (105 tests) and lintDebug green.

1. S21 — RunClaim is atomic. claim() is a compareAndSet loop (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() an updateAndGet. 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. RunClaimTest gained 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) around pageOpened(). Chosen over a pageOnScreen check inside pageOpened() 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 what SUCCESS_NAV_GRACE_MS exists 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. onFailure keeps the answer from responded() 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 — navStillWanted moved to data/follow/SuccessNav.kt, beside ProbeCadence, RunClaim and LegacyEnding. Test import followed it.

5. Nits

  • Refused claim is no longer silent. Both call sites report "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.
  • LegacyEnding is 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 reads ending == null instead of re-deriving it from progressTopic, so the two facts cannot drift.
  • Downloads have an exit. "Stop download" beside the percentage, 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's onProgress keeps firing, so a transferSeq retires its samples at cancel time (otherwise it paints over the next download's line — the same failure the run token exists to stop); and openAction now 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. runCatching catches the cancellation too, so it is rethrown and a finally clears 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 main after this lands, so the reformat is reviewable as a reformat.

Checklist posted at `db9ee59`. `./gradlew assembleDebug`, `testDebugUnitTest` (105 tests) and `lintDebug` green. **1. S21 — `RunClaim` is atomic.** `claim()` is a `compareAndSet` loop (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()` an `updateAndGet`. 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. `RunClaimTest` gained 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)` around `pageOpened()`. Chosen over a `pageOnScreen` check inside `pageOpened()` 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 what `SUCCESS_NAV_GRACE_MS` exists 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.** `onFailure` keeps the answer from `responded()` 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 — `navStillWanted` moved** to `data/follow/SuccessNav.kt`, beside `ProbeCadence`, `RunClaim` and `LegacyEnding`. Test import followed it. **5. Nits** - **Refused claim is no longer silent.** Both call sites report `"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. - **`LegacyEnding` is 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 reads `ending == null` instead of re-deriving it from `progressTopic`, so the two facts cannot drift. - **Downloads have an exit.** "Stop download" beside the percentage, `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's `onProgress` keeps firing, so a `transferSeq` retires its samples at cancel time (otherwise it paints over the next download's line — the same failure the run token exists to stop); and `openAction` now 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. `runCatching` catches the cancellation too, so it is rethrown and a `finally` clears 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 `main` after this lands, so the reformat is reviewable as a reformat.
thisilike requested changes 2026-08-13 14:54:45 +02:00
Dismissed
thisilike left a comment

Review at db9ee59. ./gradlew testDebugUnitTest green.

Every item on the #93 checklist is closed, and the two structural ones are closed properly — I checked the mechanisms, not the commit message:

  • S21. claim() is a real CAS loop with the refusal inside it, and release() is updateAndGet, 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.
  • S20. 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.
  • S22. Transport failure after "done" goes to the feed and the verdict stays; a 4xx after "done" still banners. That distinction is the right one — the server saying it failed is not the same as us being unable to tell.
  • S23 moved, test import with it. BUSY closes the silent refusal. LegacyEnding allocated only when there is a stream, and ending == null replacing action.progressTopic == null collapses "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():865 cancels downloadJob, nulls the field, bumps transferSeq. It never touches _state. Everything the operator can see is cleared in the coroutine's finally at :842-848 — and that finally cannot run until the coroutine actually unwinds.

It doesn't unwind. ApiClient.moduleDownload:195-205 copies the body inside withContext(Dispatchers.IO) through a ForwardingSource with no ensureActive() anywhere in the read loop:

val counting = object : okio.ForwardingSource(body.source()) {
    override fun read(sink: okio.Buffer, byteCount: Long): Long {  }
}
write(counting.buffer())

Job.cancel() on a coroutine blocked in a non-cancellable read does nothing until the read returns. So after the press:

  • state.transferring stays true → the "Downloading …" line and the "Stop download" button stay on screen;
  • state.downloading stays non-null → openAction:795 refuses every new transfer;
  • nothing changes until the body finishes or a single read exceeds the 30s read timeout — and a steadily-streaming slow download never trips that, so a large file on a bad link ignores the button for minutes.

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:

  1. cancelDownload() clears downloading / transferring / transferPercent itself — the same thing endRun does 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.
  2. The finally at :842 must then be guarded on seq == transferSeq.get(). Once (1) lands, the operator can start download B while A is still blocked in its read, and A's unguarded finally will clear B's transfer state. reportTransfer:1007 already 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-409 cancels refreshJobs, runJob, progressJob and calls claim.release(). It does not cancel downloadJob and does not bump transferSeq. So after retry(), an in-flight transfer's reportTransfer still matches the current seq and writes transferPercent onto the freshly reset state, and its finally later clears transfer state on a page that has none. The run lock got the full treatment in start(); the transfer got none of it.

S25 — the BUSY banner never clears. releaseRun(error) preserves actionError when error is null (actionError = if (error != null && !toForm) error else it.actionError), which is right for "a release must not wipe an unrelated failure". But BUSY is 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 in SchemaViewModel that 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.

Review at `db9ee59`. `./gradlew testDebugUnitTest` green. **Every item on the #93 checklist is closed**, and the two structural ones are closed properly — I checked the mechanisms, not the commit message: - **S21.** `claim()` is a real CAS loop with the refusal inside it, and `release()` is `updateAndGet`, 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. - **S20.** `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. - **S22.** Transport failure after "done" goes to the feed and the verdict stays; a 4xx after "done" still banners. That distinction is the right one — the server saying it failed is not the same as us being unable to tell. - **S23** moved, test import with it. `BUSY` closes the silent refusal. `LegacyEnding` allocated only when there is a stream, and `ending == null` replacing `action.progressTopic == null` collapses "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():865` cancels `downloadJob`, nulls the field, bumps `transferSeq`. It never touches `_state`. Everything the operator can see is cleared in the coroutine's `finally` at `:842-848` — and that `finally` cannot run until the coroutine actually unwinds. It doesn't unwind. `ApiClient.moduleDownload:195-205` copies the body inside `withContext(Dispatchers.IO)` through a `ForwardingSource` with no `ensureActive()` anywhere in the read loop: ```kotlin val counting = object : okio.ForwardingSource(body.source()) { override fun read(sink: okio.Buffer, byteCount: Long): Long { … } } write(counting.buffer()) ``` `Job.cancel()` on a coroutine blocked in a non-cancellable read does nothing until the read returns. So after the press: - `state.transferring` stays true → the "Downloading …" line and the **"Stop download" button stay on screen**; - `state.downloading` stays non-null → `openAction:795` refuses every new transfer; - nothing changes until the body finishes or a *single read* exceeds the 30s read timeout — and a steadily-streaming slow download never trips that, so a large file on a bad link ignores the button for minutes. 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: 1. `cancelDownload()` clears `downloading` / `transferring` / `transferPercent` itself — the same thing `endRun` does 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. 2. The `finally` at `:842` must then be guarded on `seq == transferSeq.get()`. Once (1) lands, the operator can start download B while A is still blocked in its read, and A's unguarded `finally` will clear B's transfer state. `reportTransfer:1007` already 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-409` cancels `refreshJobs`, `runJob`, `progressJob` and calls `claim.release()`. It does not cancel `downloadJob` and does not bump `transferSeq`. So after `retry()`, an in-flight transfer's `reportTransfer` still matches the current seq and writes `transferPercent` onto the freshly reset state, and its `finally` later clears transfer state on a page that has none. The run lock got the full treatment in `start()`; the transfer got none of it. **S25 — the `BUSY` banner never clears.** `releaseRun(error)` preserves `actionError` when `error` is null (`actionError = if (error != null && !toForm) error else it.actionError`), which is right for "a release must not wipe an unrelated failure". But `BUSY` is 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 in `SchemaViewModel` that 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.
"Stop download" cancelled the job and nothing else. The bytes are read
blocking through a ForwardingSource with no cancellation check, so the
coroutine did not unwind — and its finally, which cleared every visible
field, could not run — until the body ended or a single read timed out.
A steadily-streaming slow download never reaches that, so the button,
the "Downloading …" line and the refusal of every new transfer stayed on
screen for minutes after the press.

cancelDownload now clears downloading/transferPercent/transferring
itself, as endRun does for the run lock: the exit is what frees the
screen, not a consequence that arrives when the work ends anyway. The
abandoned coroutine's own teardown — and its success tail, the "Saved …"
line and the viewer it opens — is gated on the transfer seq, the check
reportTransfer already carried for the write path, so it cannot clear or
paint over a download started in the meantime.

start() joins that teardown: it reset the page while a transfer ran, so
the in-flight download's seq stayed current, wrote percentages onto the
fresh state, and later cleared a transfer the page no longer had.

releasedBanner: the busy notice does not outlive the lock it describes.
Every other banner still survives a release; BUSY is a statement about
the lock, and the release is the lock opening.

3 tests (108).
Author
Owner

Pushed a2df357. ./gradlew assembleDebug, testDebugUnitTest (108) and lintDebug green.

B9 — the exit frees the screen. Both halves, as prescribed.

cancelDownload() clears downloading / transferPercent / transferring itself. 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 the finally could not run until the body ended — and the diagnosis (ForwardingSource, no ensureActive(), 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 finally is gated on seq == transferSeq.get(), the same check reportTransfer carries. 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 its actionError onto 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() in ApiClient.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() calls cancelDownload(), 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 — except BUSY, which is a statement about the lock the release just opened. ReleasedBannerTest pins 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.

Pushed `a2df357`. `./gradlew assembleDebug`, `testDebugUnitTest` (108) and `lintDebug` green. **B9 — the exit frees the screen.** Both halves, as prescribed. `cancelDownload()` clears `downloading` / `transferPercent` / `transferring` itself. 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 the `finally` could not run until the body ended — and the diagnosis (`ForwardingSource`, no `ensureActive()`, 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 `finally` is gated on `seq == transferSeq.get()`, the same check `reportTransfer` carries. 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 its `actionError` onto 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()` in `ApiClient.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()` calls `cancelDownload()`, 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 — except `BUSY`, which is a statement about the lock the release just opened. `ReleasedBannerTest` pins 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.
thisilike requested changes 2026-08-13 15:11:50 +02:00
Dismissed
thisilike left a comment

Review at a2df357. ./gradlew testDebugUnitTest green.

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_VIEW Intent minutes after the operator walked away. releasedBanner as a pinned function with BUSY as 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:1023 streams into a temp path derived only from the URL:

val temp = File(dir, ".$fallbackName.part")

temp.sink().buffer().use { sink -> sink.writeAll(source) }

Before this commit that was safe by accident: downloading stayed non-null after a cancel, so no second transfer could start. cancelDownload():913 now frees the guard the moment it is pressed, while the abandoned coroutine is still blocked inside moduleDownload's read loop. Press the same action again — the obvious thing to do after cancelling a stalled download — and two transfers open .<name>.part and writeAll into it concurrently.

Whichever finishes first renames it to a uniqueName final 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. uniqueName guards the final name and nothing guards the temp one.

Q+ is fine — each transfer does its own resolver.insert and 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. cancelDownload cancels a coroutine that cannot observe cancellation: ApiClient.moduleDownload reads through a plain ForwardingSource, and withContext(Dispatchers.IO) only throws once the blocking read returns on its own. It also sets downloadJob = null, so openAction:816 re-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 Call and cancelling it — okHttp.newCall(req) held, currentCoroutineContext().job.invokeOnCompletion { call.cancel() } around the withContext, or an isActive check inside the ForwardingSource.read override — 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

  • releasedBanner and BUSY are pure, internal, and now pinned by a test — and they live in SchemaViewModel.kt, while navStillWanted was moved to data/follow/ last round for precisely that reason. One rule or the other.
  • ReleasedBannerTest pins 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() calls cancelDownload() and then _state.value = SchemaUiState(), so the state write inside cancelDownload is immediately superseded. Harmless, just dead.
  • A transfer cancelled at the last instant still lands its file, and the seq guard now suppresses the "Saved …" line — so the file is in Downloads and the app says nothing about it. Defensible; worth being a decision rather than a consequence of where the guard landed.

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 `a2df357`. `./gradlew testDebugUnitTest` green. 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_VIEW` Intent minutes after the operator walked away. `releasedBanner` as a pinned function with `BUSY` as 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:1023` streams into a temp path derived only from the URL: ```kotlin val temp = File(dir, ".$fallbackName.part") … temp.sink().buffer().use { sink -> sink.writeAll(source) } ``` Before this commit that was safe by accident: `downloading` stayed non-null after a cancel, so no second transfer could start. `cancelDownload():913` now frees the guard the moment it is pressed, while the abandoned coroutine is still blocked inside `moduleDownload`'s read loop. Press the same action again — the obvious thing to do after cancelling a stalled download — and two transfers open `.<name>.part` and `writeAll` into it concurrently. Whichever finishes first renames it to a `uniqueName` final 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. `uniqueName` guards the final name and nothing guards the temp one. Q+ is fine — each transfer does its own `resolver.insert` and 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.** `cancelDownload` cancels a coroutine that cannot observe cancellation: `ApiClient.moduleDownload` reads through a plain `ForwardingSource`, and `withContext(Dispatchers.IO)` only throws once the blocking read returns on its own. It also sets `downloadJob = null`, so `openAction:816` re-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 `Call` and cancelling it — `okHttp.newCall(req)` held, `currentCoroutineContext().job.invokeOnCompletion { call.cancel() }` around the `withContext`, or an `isActive` check inside the `ForwardingSource.read` override — 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 - `releasedBanner` and `BUSY` are pure, `internal`, and now pinned by a test — and they live in `SchemaViewModel.kt`, while `navStillWanted` was moved to `data/follow/` last round for precisely that reason. One rule or the other. - `ReleasedBannerTest` pins 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()` calls `cancelDownload()` and then `_state.value = SchemaUiState()`, so the state write inside `cancelDownload` is immediately superseded. Harmless, just dead. - A transfer cancelled at the last instant still lands its file, and the seq guard now suppresses the "Saved …" line — so the file is in Downloads and the app says nothing about it. Defensible; worth being a decision rather than a consequence of where the guard landed. ## 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.
B10 — the pre-Q download path streamed into `.<name>.part`, a path
derived only from the URL. That was safe only while `downloading` stayed
set after a cancel; now that the exit frees the guard immediately, the
obvious next move — press the same action again — put two transfers on
one inode. The winner renamed it to a unique final name and the loser's
fd followed the inode, so the app reported a saved file whose bytes were
not what the server sent. The seq was already threaded into both
download functions; the temp name now carries it. Q+ was never affected:
each transfer inserts its own MediaStore row.

S26 — `moduleDownload` holds its `Call` and cancels it when its
coroutine completes, so cancelling a download actually ends it. The copy
runs through a plain `ForwardingSource`, which no coroutine machinery
can interrupt: an abandoned read held an IO thread and a connection
until a single read exceeded the read timeout, which a slow but steady
stream never reaches, and every cancel-and-retry stacked another one.
Closing the socket is what the read is waiting on. The handle is
disposed in a `finally` so a completed download leaves nothing on the
job.

That also makes the exit honest about the file: a cancelled transfer now
fails its read, and both paths already delete their partial file on
failure. One consequence stays — a transfer cancelled in the last
instant before the rename keeps its file and says nothing, which the
KDoc now argues for rather than leaving as an accident of where the seq
guard fell. So does the other: a `.part` orphaned by a process kill is
now one per transfer rather than one per name, in a directory that goes
on uninstall.

Nits from the same round: `releasedBanner`/`BUSY` moved to
`data/follow/` next to `navStillWanted`, one rule for where a pinned
pure function lives; `releasedBanners` extracts the routing expression
the ViewModel held inline, which is the half that decides which banner
an operator reads the failure in and the half no test covered;
`abandonDownload` splits the job/seq retirement from the screen write,
so `start()` no longer writes state it replaces a line later.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thisilike requested changes 2026-08-13 16:01:07 +02:00
Dismissed
thisilike left a comment

Review at 08eddbf. ./gradlew testDebugUnitTest green.

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 the dispose() in finally keeps a completed transfer from cancelling its own finished call. releasedBanners + Banners moved to data/follow/ with the routing now pinned by tests (including the both-sides BUSY case, which is the one I would have forgotten). abandonDownload/cancelDownload splits the dead write out of start().

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:1028 is now:

val temp = File(dir, ".$fallbackName.$seq.part")

seq comes from transferSeq (:331), which is private val transferSeq = AtomicInteger(0)an instance field of SchemaViewModel, 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 a uniqueName final name and the loser's fd keeps appending into the file the operator was just handed.

fallbackName is fileNameOf(path), a basename off a module-relative route, so two modules serving config.yaml or backup.tar.gz is 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, and seq goes back to being purely an ownership token. Q+ is unaffected — every transfer does its own resolver.insert and gets its own URI.

Should fix

S27 — the cancellable-request fix landed on the download and not on the upload. moduleDownload:179 holds its Call and cancels it; modulePostMultipart:130 is still a bare okHttp.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

  • Orphaned .part files 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.part behind for good, and nothing ever sweeps the directory. createTempFile makes this worse, not better, so it is worth a dir.listFiles { … endsWith(".part") } cleanup on start() — or at least an acknowledgement that this directory grows.
  • The invokeOnCompletion comment says the dispose exists "so a completed download does not leave a handler on a job that outlives it". The job is the withContext(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 fires call.cancel() on its own completion. Right code, wrong reason on the label.
  • BUSY is user-facing English living in data/follow/. Consistent with the rest of the codebase (no string resources anywhere), so this is a note rather than a request — but a data package is where it will be least expected when localisation eventually happens.

Standing

Seven units extracted and pinned; none produced a finding this round, and ReleasedBannerTest in 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.

Review at `08eddbf`. `./gradlew testDebugUnitTest` green. 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 the `dispose()` in `finally` keeps a completed transfer from cancelling its own finished call. `releasedBanners` + `Banners` moved to `data/follow/` with the routing now pinned by tests (including the both-sides `BUSY` case, which is the one I would have forgotten). `abandonDownload`/`cancelDownload` splits the dead write out of `start()`. 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:1028` is now: ```kotlin val temp = File(dir, ".$fallbackName.$seq.part") ``` `seq` comes from `transferSeq` (`:331`), which is `private val transferSeq = AtomicInteger(0)` — **an instance field of `SchemaViewModel`**, 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 a `uniqueName` final name and the loser's fd keeps appending into the file the operator was just handed. `fallbackName` is `fileNameOf(path)`, a basename off a module-relative route, so two modules serving `config.yaml` or `backup.tar.gz` is 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, and `seq` goes back to being purely an ownership token. Q+ is unaffected — every transfer does its own `resolver.insert` and gets its own URI. ## Should fix **S27 — the cancellable-request fix landed on the download and not on the upload.** `moduleDownload:179` holds its `Call` and cancels it; `modulePostMultipart:130` is still a bare `okHttp.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 - **Orphaned `.part` files 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.part` behind for good, and nothing ever sweeps the directory. `createTempFile` makes this worse, not better, so it is worth a `dir.listFiles { … endsWith(".part") }` cleanup on `start()` — or at least an acknowledgement that this directory grows. - The `invokeOnCompletion` comment says the dispose exists "so a completed download does not leave a handler on a job that outlives it". The job is the `withContext(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 fires `call.cancel()` on its own completion. Right code, wrong reason on the label. - `BUSY` is user-facing English living in `data/follow/`. Consistent with the rest of the codebase (no string resources anywhere), so this is a note rather than a request — but a `data` package is where it will be least expected when localisation eventually happens. ## Standing Seven units extracted and pinned; none produced a finding this round, and `ReleasedBannerTest` in 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.
B11: the per-transfer temp name was unique within one ViewModel, and the
directory is per-device. `transferSeq` is an instance counter, one
ViewModel per module/subPath, several alive on the back stack, each
counting from 0 — so two pages downloading the same basename both wrote
`.backup.tar.gz.1.part`, one published it under a `uniqueName` final
name, and the loser's fd went on appending to the file just handed to
the operator. `File.createTempFile` takes the name from the OS, which is
what owns the contended resource. `transferSeq` goes back to being
purely an ownership token, and says so.

Orphans are the price of that: a fixed name left at most one stale
partial per download, a per-transfer name leaves a new one every process
death. `start` sweeps them, on age rather than ownership — a live
transfer writes, and writing moves the modification time, so an
untouched-for-a-day partial has no writer left to rob of its rename.
`isOrphanPart` is the rule, pure and pinned, because it deletes files.

S27: the cancellable-request fix had landed on the download and not on
the upload, which is the longer of the two waits and the one "Stop
waiting" is offered against. `modulePostMultipart` holds its `Call` and
cancels it on completion, and the submit coroutine is tracked as
`submitJob` so `endRun` and a page turn can end the request and not only
the wait for it — cancelling is safe because every continuation below is
already gated on the claim token. `abandonRun`'s KDoc no longer claims
the request survives.

The `invokeOnCompletion` dispose was right code with the wrong reason on
the label: nothing outlives anything, it exists so a *successful*
transfer does not fire `call.cancel()` on its own completion.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Owner

Pushed bad6e49. ./gradlew assembleDebug, testDebugUnitTest (112) and lintDebug green.

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: transferSeq is 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, createTempFile can 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 why fileNameOf never 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 createTempFile makes 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 in DownloadNamingTest beside uniqueName, because it is a predicate that deletes files. lastModified() == 0 is 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. modulePostMultipart holds its Call and cancels it on completion, same as moduleDownload. But the submit coroutine was untracked: viewModelScope.launch with nobody holding the Job, so there was nothing for the exit to cancel. It is submitJob now, cancelled by endRun (every exit, "Stop waiting" included) and by start() — 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. followRun launches on viewModelScope, 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.

  • The invokeOnCompletion label: 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 fire call.cancel() on a transfer that just succeeded.
  • BUSY in data/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 versionName stays 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 into isOrphanPart rather than into the coroutine, which is the eighth unit extracted for that reason.

🤖 Generated with Claude Code

Pushed `bad6e49`. `./gradlew assembleDebug`, `testDebugUnitTest` (112) and `lintDebug` green. **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: `transferSeq` is 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, `createTempFile` can 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 why `fileNameOf` never 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 `createTempFile` makes 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 in `DownloadNamingTest` beside `uniqueName`, because it is a predicate that deletes files. `lastModified() == 0` is 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.** `modulePostMultipart` holds its `Call` and cancels it on completion, same as `moduleDownload`. But the submit coroutine was untracked: `viewModelScope.launch` with nobody holding the `Job`, so there was nothing for the exit to cancel. It is `submitJob` now, cancelled by `endRun` (every exit, "Stop waiting" included) and by `start()` — 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. `followRun` launches on `viewModelScope`, 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.** - The `invokeOnCompletion` label: 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 fire `call.cancel()` on a transfer that just succeeded. - `BUSY` in `data/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 `versionName` stays 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 into `isOrphanPart` rather than into the coroutine, which is the eighth unit extracted for that reason. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
thisilike requested changes 2026-08-13 18:04:07 +02:00
Dismissed
thisilike left a comment

Review at bad6e49. ./gradlew testDebugUnitTest green.

B11 and S27 are closed on the paths they name. File.createTempFile is 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:253 ends in ?: "download" and sanitiseFilename never returns empty, so the shortest possible prefix is .X. and the three-character floor holds. isOrphanPart guards both lastModified == 0 and a clock that moved backwards, and both are tested. The invokeOnCompletion comment now gives the real reason for its dispose().

The sweep those temp names made necessary deletes saved downloads.

Blocking

B12 — sweepOrphanParts deletes the operator's files

isOrphanPart:307 decides on the name alone:

name.endsWith(".part") && lastModifiedMs > 0 && nowMs - lastModifiedMs >= PART_ORPHAN_MS

and sweepOrphanParts:476 runs it over getExternalFilesDir(DIRECTORY_DOWNLOADS)the same directory finished downloads are saved into. downloadToAppDownloads:1135 renames the temp to File(dir, finalName) in that exact directory, and finalName comes from the server: content-disposition, or the URL basename, through sanitiseFilename and uniqueName, 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 ships module_filebrowser — a module whose whole job is letting the operator download arbitrary files off the server — so "a file named something.part" is not a hypothetical, it is a category.

The temp files all begin with . (the createTempFile prefix at :1120 is ".$fallbackName."), so name.startsWith(".") && name.endsWith(".part") is the one-line version. The robust version is a subdirectory — write partials into dir/.parts/ and sweep only that — which makes it structurally impossible for the sweep to see a saved file at all, including the .foo.part case the dot check still misses.

DownloadNamingTest reinforces the gap rather than catching it: its "not a partial" case is backup.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:130 and moduleDownload:179 both hold their Call and cancel it. modulePost:100 is still a bare okHttp.newCall(req).execute().use { … }, and runAction has no job field at all — only submitJob:363 exists, cancelled in endRun and start().

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: LegacyEnding exists 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

  • submitJob is never nulled on normal completion, so a finished Job stays referenced until the next submit. Trivial, but runJob/progressJob/downloadJob all null themselves and this one does not.
  • The sweep runs on every 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_MS and isOrphanPart are new pure, pinned rules that stayed in SchemaViewModel.kt, while navStillWanted and releasedBanner were moved to data/follow/ for being exactly that. Defensible — uniqueName and fileNameOf already 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.

Review at `bad6e49`. `./gradlew testDebugUnitTest` green. B11 and S27 are closed on the paths they name. `File.createTempFile` is 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:253` ends in `?: "download"` and `sanitiseFilename` never returns empty, so the shortest possible prefix is `.X.` and the three-character floor holds. `isOrphanPart` guards both `lastModified == 0` and a clock that moved backwards, and both are tested. The `invokeOnCompletion` comment now gives the real reason for its `dispose()`. The sweep those temp names made necessary deletes saved downloads. ## Blocking ### B12 — `sweepOrphanParts` deletes the operator's files `isOrphanPart:307` decides on the name alone: ```kotlin name.endsWith(".part") && lastModifiedMs > 0 && nowMs - lastModifiedMs >= PART_ORPHAN_MS ``` and `sweepOrphanParts:476` runs it over `getExternalFilesDir(DIRECTORY_DOWNLOADS)` — **the same directory finished downloads are saved into**. `downloadToAppDownloads:1135` renames the temp to `File(dir, finalName)` in that exact directory, and `finalName` comes from the server: `content-disposition`, or the URL basename, through `sanitiseFilename` and `uniqueName`, 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 ships `module_filebrowser` — a module whose whole job is letting the operator download arbitrary files off the server — so "a file named `something.part`" is not a hypothetical, it is a category. The temp files all begin with `.` (the `createTempFile` prefix at `:1120` is `".$fallbackName."`), so `name.startsWith(".") && name.endsWith(".part")` is the one-line version. The robust version is a subdirectory — write partials into `dir/.parts/` and sweep only that — which makes it structurally impossible for the sweep to see a saved file at all, including the `.foo.part` case the dot check still misses. `DownloadNamingTest` reinforces the gap rather than catching it: its "not a partial" case is `backup.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:130` and `moduleDownload:179` both hold their `Call` and cancel it. `modulePost:100` is still a bare `okHttp.newCall(req).execute().use { … }`, and `runAction` has no job field at all — only `submitJob:363` exists, cancelled in `endRun` and `start()`. 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: `LegacyEnding` exists 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 - `submitJob` is never nulled on normal completion, so a finished `Job` stays referenced until the next submit. Trivial, but `runJob`/`progressJob`/`downloadJob` all null themselves and this one does not. - The sweep runs on every `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_MS` and `isOrphanPart` are new pure, pinned rules that stayed in `SchemaViewModel.kt`, while `navStillWanted` and `releasedBanner` were moved to `data/follow/` for being exactly that. Defensible — `uniqueName` and `fileNameOf` already 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.
B12: the orphan sweep walked the downloads directory itself and decided by
name, so a saved file ending in ".part" - a category, not a hypothetical,
with module_filebrowser serving arbitrary files - was silently deleted a
day after landing. Partials now live in a ".parts" subdirectory whose only
exit is the rename at the end of a transfer; the sweep reads that directory
and nothing else, which makes deleting a saved file structurally impossible
rather than one naming convention away. Pinned by a test whose saved file
is named exactly like a partial.

S28: modulePost holds its Call and cancels it on the caller's completion,
same as its multipart and download twins, and runAction's job is tracked
(actionJob) and cancelled by endRun/start - so "Stop waiting" on an action
now ends the request instead of leaving a blocking read holding an IO
thread and a connection until a timeout. That is the long case, not the
short one: a LegacyEnding module does its work inside its own request.
abandonRun's KDoc gets to say "request" again.

Nits: submitJob and actionJob null themselves on completion; the sweep
runs once per process (orphans are made by process death, so that is how
often new ones can appear), handing the flag back if its launch is
cancelled before the dispatcher runs it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

b1c13f5 answers #101. ./gradlew testDebugUnitTest and assembleDebug green.

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): createTempFile targets that subdirectory, and the rename at the end of a transfer is the only way a file leaves it. The traversal moved into a pure sweepParts(partsDir, nowMs) that reads that directory and nothing else — so whatever isOrphanPart says about a name, a saved file one directory up is unreachable, .foo.part included. The name check stays inside isOrphanPart as 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 than PART_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

modulePost holds its Call and cancels it from invokeOnCompletion, disposed in finally — the same mechanism as its multipart and download twins, for the same reason. runAction tracks its job (actionJob), and endRun / start() cancel it alongside submitJob. So "Stop waiting" on an action ends the request itself, which matters most for exactly the case you named: a LegacyEnding module doing its work inside its own POST. abandonRun's KDoc says "request" again, now covering both write paths.

Nits

  • submitJob and actionJob null themselves on completion (invokeOnCompletion with an === guard, so a job that was already replaced does not null its successor).
  • The sweep runs once per process (partsSwept, file-level AtomicBoolean) — 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.
  • The file-placement nit stays as is, on your own "defensible" — download naming keeps its home next to uniqueName and fileNameOf.

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.

`b1c13f5` answers #101. `./gradlew testDebugUnitTest` and `assembleDebug` green. ## 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`): `createTempFile` targets that subdirectory, and the rename at the end of a transfer is the only way a file leaves it. The traversal moved into a pure `sweepParts(partsDir, nowMs)` that reads that directory and nothing else — so whatever `isOrphanPart` says about a name, a saved file one directory up is unreachable, `.foo.part` included. The name check stays inside `isOrphanPart` as 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 than `PART_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 `modulePost` holds its `Call` and cancels it from `invokeOnCompletion`, disposed in `finally` — the same mechanism as its multipart and download twins, for the same reason. `runAction` tracks its job (`actionJob`), and `endRun` / `start()` cancel it alongside `submitJob`. So "Stop waiting" on an action ends the request itself, which matters most for exactly the case you named: a `LegacyEnding` module doing its work inside its own POST. `abandonRun`'s KDoc says "request" again, now covering both write paths. ## Nits - `submitJob` and `actionJob` null themselves on completion (`invokeOnCompletion` with an `===` guard, so a job that was already replaced does not null its successor). - The sweep runs once per process (`partsSwept`, file-level `AtomicBoolean`) — 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. - The file-placement nit stays as is, on your own "defensible" — download naming keeps its home next to `uniqueName` and `fileNameOf`. 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.
thisilike approved these changes 2026-08-13 23:02:24 +02:00
thisilike left a comment

Review at b1c13f5. ./gradlew testDebugUnitTest green. 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_DIR puts partials one directory down and sweepParts is scoped to it, so a saved file is not "one naming convention away" from the sweep — it is unreachable by it. sweepParts extracted as a pure traversal over a File is the right seam, and the test is the one I would have written: a saved something.part survives a sweep that deletes a real orphan and leaves a live partial alone.
  • The rename still works across the two directories (same volume), and 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.
  • partsSwept as a process-wide AtomicBoolean matches 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.
  • modulePost now cancels its Call, actionJob is tracked and cancelled in both endRun and start(), 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 fold still runs (its runCatching swallows the CancellationException), and every branch is behind claim.owns(token), which releaseRun has 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.

val finalName = uniqueName(name) { File(dir, it).exists() }
val target = File(dir, finalName)
if (!temp.renameTo(target)) error()

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 finalName neither exists() at check time, so both compute the same candidate and the second renameTo silently replaces the first. That is exactly what uniqueName'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. minSdk is 26, so Files.move(temp.toPath(), target.toPath()) without REPLACE_EXISTING throws FileAlreadyExistsException atomically, and retrying with the next candidate makes the loop a claim instead of a poll.

Nits

  • runAction and submitForm wrap their POST in runCatching without rethrowing CancellationException, while probe() and openAction both do. Harmless here — the token guard catches it — but the rule this PR established has two exceptions left in it.
  • downloadJob is now the only job field without the self-nulling invokeOnCompletion the other two just got. Functionally fine (isActive is false on a completed job), but it is the odd one out.
  • The sweep's KDoc says Q+ "deletes its own pending row on failure". True for failures it sees; process death leaves a pending IS_PENDING row 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.
  • Partials written directly into the downloads directory by earlier builds of this branch are now outside the sweep's reach forever. Only affects machines that ran the intermediate commits.

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 — runningAction across 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 was RunClaim, 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.

SchemaViewModel still 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.

Review at `b1c13f5`. `./gradlew testDebugUnitTest` green. **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_DIR` puts partials one directory down and `sweepParts` is scoped to it, so a saved file is not "one naming convention away" from the sweep — it is unreachable by it. `sweepParts` extracted as a pure traversal over a `File` is the right seam, and the test is the one I would have written: a saved `something.part` survives a sweep that deletes a real orphan and leaves a live partial alone. - The rename still works across the two directories (same volume), and `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`. - `partsSwept` as a process-wide `AtomicBoolean` matches 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. - `modulePost` now cancels its `Call`, `actionJob` is tracked and cancelled in both `endRun` and `start()`, 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 `fold` still runs (its `runCatching` swallows the `CancellationException`), and every branch is behind `claim.owns(token)`, which `releaseRun` has 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.** ```kotlin val finalName = uniqueName(name) { File(dir, it).exists() } val target = File(dir, finalName) if (!temp.renameTo(target)) error(…) ``` 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 `finalName` neither `exists()` at check time, so both compute the same candidate and the second `renameTo` silently replaces the first. That is exactly what `uniqueName`'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. `minSdk` is 26, so `Files.move(temp.toPath(), target.toPath())` without `REPLACE_EXISTING` throws `FileAlreadyExistsException` atomically, and retrying with the next candidate makes the loop a claim instead of a poll. ## Nits - `runAction` and `submitForm` wrap their POST in `runCatching` without rethrowing `CancellationException`, while `probe()` and `openAction` both do. Harmless here — the token guard catches it — but the rule this PR established has two exceptions left in it. - `downloadJob` is now the only job field without the self-nulling `invokeOnCompletion` the other two just got. Functionally fine (`isActive` is false on a completed job), but it is the odd one out. - The sweep's KDoc says Q+ "deletes its own pending row on failure". True for failures it sees; process death leaves a pending `IS_PENDING` row 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. - Partials written directly into the downloads directory by earlier builds of this branch are now outside the sweep's reach forever. Only affects machines that ran the intermediate commits. ## 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 — `runningAction` across 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 was `RunClaim`, `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. `SchemaViewModel` still 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.
julian merged commit 31461a4fac into main 2026-08-13 23:10:40 +02:00
julian deleted branch feat/task-runs 2026-08-13 23:10:40 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/mobile!4
No description provided.