Install queue: failures never block, cancel and retry (#74) #76

Merged
claude-bot merged 3 commits from fix/install-queue into main 2026-10-23 15:46:59 +00:00
Collaborator

What

Refs #74. The GerSch download failed with "Temporary failure in name resolution" and everything else waited on it.
Root cause: the click's confined app had no network access. utstudybible.apparmor had "policy_groups": [] (since the repo-setup commit 9f9beba), but D10 and docs/07 say the app needs networking. It now lists it.
Second bug, the softlock itself: Transfer::run retried network errors for ever inside the single-threaded runner, so one failing download held the whole queue, and no cancel existed. Retries now end, the item is marked failed, the next one installs, and Cancel, Cancel all and per-row Retry exist on the progress view.
A failed download keeps its bytes for the retry; a cancel during unpacking removes the module; orphaned files in the tasks folder are swept at start.

Evidence for the root cause (phone, read-only ssh):

  • /var/lib/apparmor/profiles/click_utstudybible.schaefera_utstudybible_0.0.1 has no network rule and no nameservice abstraction. A grep for network|inet|nameservice finds only the openssl abstraction and unrelated comments. Other click profiles (calendar, dekko) list "networking" and contain #include <abstractions/nameservice>.
  • The journal shows install GerSch downloading then retrying in the same second on every launch (16:40:39, 16:41:38, 16:44:33). That fits an immediate EAI_AGAIN from getaddrinfo in the confined process, while the unconfined shell resolves ftp.crosswire.org fine.
  • app.sqlite3 (copied off) shows GerSch running and the other five queued; the tasks dir has an empty GerSch.zip.part.
  • Ruled out: a stale error (each launch fails fresh), IPv6 or NAT64 handling (the lookup fails before any address is used), the HTTP client (ureq with rustls, plain getaddrinfo).
  • Not directly seen: an AppArmor DENIED line for the socket (the journal shows only dbus denials; network denials may not be logged). The proof is the profile contents plus the symptom, not a denial record. The owner should still confirm on the phone that GerSch installs with the networking group.

Blast radius

Tier: trunk
Trunk paths touched: sword/src/install*, core/src/tasks/, qt/ glue, qml/Main.qml, *.apparmor, tools/e2e/
Testing tier: standard (also: config, the AppArmor policy, which only the device enforces)
Gate / rollback: Revert the merge commit of PR #76. No migration was added; the schema is unchanged and the Db change is one new method, so nothing is forward-only. Reverting also puts "policy_groups": [] back, which removes the network access; a reinstalled older click would hit the same DNS failure. The user's data is untouched: tasks rows created by the new code are ordinary failed rows, and the old code reads them fine.

Before leaving draft, per docs/09 "On the phone": install the dev click on Plinius and check that GerSch downloads. This is the one thing the CI cannot prove. Also check Cancel and Cancel all on the progress view.

Owner phone try-list:

  • an unreachable source fails one item and the rest install
  • cancel a queued, a running and a failed item
  • retry a failed item
  • kill mid-download and reopen
  • install a Bible after all that

Proof

  • Gates, green on head 60d11c7 (later commits 6347524 and c6ef183 added the keep-bytes fix, wake-after-send and an e2e tweak; cargo make was green locally before the push, and CI ran app and e2e on the pushed head).
  • cargo make: tests 69+8+3+3+170+15+4 passed, 0 failed; coverage TOTAL lines 94.80% (regions 94.50%); deny and audit ok.
  • cargo make app-docker: EXIT 0. "translations: po/ is up to date with the sources", "utsb: 70 translated messages". mem-check normal: PSS 151421 kB (budget 165000 kB), slow: 151969 kB.
  • cargo make e2e-docker: "e2e: 21 passed, 0 failed, 0 skipped" and, slow profile, "e2e: 4 passed, 0 failed, 0 skipped". New: PASS install-queue-fail (23s): BSB answers 503 and is marked failed ("the server answered HTTP 503"), TSK installs, then Retry on the row installs BSB. PASS install-queue-cancel (29s): a queued row is cancelled and never fetched, Cancel all stops the running download, the tasks dir is empty, the choice is back and an install works.
  • FAIL without the change, e2e against origin/main with the new case copied in: FAIL install-queue-fail (100s), install-queue-fail: FAIL: no 'utsb: install BSB failed' (x1) after 90 s, e2e: 0 passed, 1 failed. On base the 503 source is retried for ever and TSK never installs.
  • Unit tests, each run with a deliberate break (change broken, test failing):
    • No retry limit, sword: test install::http::tests::a_source_that_keeps_failing_ends_the_download ... FAILED
    • No retry limit, core: test tasks::tests::a_source_that_keeps_failing_fails_its_task_and_the_next_one_installs ... FAILED (GerSch retries 3 times, then installs "Done", so no failure is ever reported)
    • No wake: test tasks::tests::a_cancel_does_not_wait_for_the_backoff ... FAILED
    • CancelAll a no-op: cancel_all_also_forgets_a_failed_item ... FAILED and cancel_all_clears_the_queue_the_running_download_and_the_failed ... FAILED
    • Cancel keeps failed rows: a_failed_item_can_be_cancelled_and_stays_gone ... FAILED and cancel_all_also_forgets_a_failed_item ... FAILED
    • No orphan sweep: files_left_for_a_task_that_is_gone_are_swept_at_start ... FAILED
    • Board cancel keeps the row: tasks::progress::tests::a_cancelled_row_goes_and_late_events_for_it_change_nothing ... FAILED
    • AppArmor with "policy_groups": [], i.e. the origin/main file: test tasks::tests::the_click_may_use_the_network ... FAILED
  • Passing-only tests that guard existing behaviour: bytes_arriving_start_the_count_of_failures_again, a_wake_during_a_retry_wait_is_reported.
  • New tests: a_download_that_gave_up_keeps_its_bytes_for_the_retry (a retry after give-up asks for the Range from the kept length and installs), a_failed_item_dismissed_takes_its_bytes, a_stopped_download_s_late_word_does_not_start_a_new_row. Two existing board tests were adjusted to send Started first.
  • PSS, measured by mem-check (CI amd64, Xvfb, not the phone). Base origin/main: normal 149641 kB, slow 151135 kB. This branch: normal 151421 kB, slow 151969 kB; an earlier run of the same code gave 154256 and 155676 kB. The spread between runs (about 4 MB) is larger than the difference to base, so no measurable effect; the 165000 kB budget holds. Added resident state is one Arc and a few enum arms. The two row buttons exist only while the progress page is open and rows are listed. Start-up work is unchanged, apart from one read_dir of the tasks folder (empty in the normal case) for the orphan sweep.
  • Deviation: one write on the phone. I copied app.sqlite3* to the phone's /tmp to read it off, then removed those copies. Nothing else was touched; the app was not killed or launched.
  • I did not run app-docker or e2e-docker locally, because nothing in qt/ or QML changed in the last commits; CI ran both and they passed.

Review

Reviewer: fresh-context code-review, review #257 on this PR, with responses in the thread; open findings: none.
Review-Confidence: 70/100, copied unadjusted from the audit, with its deductions (the AppArmor effect cannot be exercised off the device).

Confidence by part

  • Retry limit, failure handling, cancel and retry in sword/src/install and core/src/tasks: high. Each behaviour has a unit test that fails when the change is broken, plus two e2e cases.
  • QML buttons and qt/ glue: medium. Covered by e2e, not tried on the phone.
  • AppArmor networking group: medium. The diagnosis rests on the profile contents and the symptom, with no DENIED line; only the phone check proves it. A human should read this part and try it on the device.

🤖 Generated with Claude Code

## What Refs #74. The GerSch download failed with "Temporary failure in name resolution" and everything else waited on it. Root cause: the click's confined app had no network access. `utstudybible.apparmor` had `"policy_groups": []` (since the repo-setup commit 9f9beba), but D10 and docs/07 say the app needs `networking`. It now lists it. Second bug, the softlock itself: `Transfer::run` retried network errors for ever inside the single-threaded runner, so one failing download held the whole queue, and no cancel existed. Retries now end, the item is marked failed, the next one installs, and Cancel, Cancel all and per-row Retry exist on the progress view. A failed download keeps its bytes for the retry; a cancel during unpacking removes the module; orphaned files in the tasks folder are swept at start. Evidence for the root cause (phone, read-only ssh): - `/var/lib/apparmor/profiles/click_utstudybible.schaefera_utstudybible_0.0.1` has no `network` rule and no nameservice abstraction. A grep for network|inet|nameservice finds only the openssl abstraction and unrelated comments. Other click profiles (calendar, dekko) list "networking" and contain `#include <abstractions/nameservice>`. - The journal shows `install GerSch downloading` then `retrying` in the same second on every launch (16:40:39, 16:41:38, 16:44:33). That fits an immediate EAI_AGAIN from getaddrinfo in the confined process, while the unconfined shell resolves ftp.crosswire.org fine. - app.sqlite3 (copied off) shows GerSch `running` and the other five `queued`; the tasks dir has an empty GerSch.zip.part. - Ruled out: a stale error (each launch fails fresh), IPv6 or NAT64 handling (the lookup fails before any address is used), the HTTP client (ureq with rustls, plain getaddrinfo). - Not directly seen: an AppArmor DENIED line for the socket (the journal shows only dbus denials; network denials may not be logged). The proof is the profile contents plus the symptom, not a denial record. The owner should still confirm on the phone that GerSch installs with the networking group. ## Blast radius **Tier:** trunk **Trunk paths touched:** `sword/src/install*`, `core/src/tasks/`, `qt/` glue, `qml/Main.qml`, `*.apparmor`, `tools/e2e/` **Testing tier:** standard (also: config, the AppArmor policy, which only the device enforces) **Gate / rollback:** Revert the merge commit of PR #76. No migration was added; the schema is unchanged and the Db change is one new method, so nothing is forward-only. Reverting also puts `"policy_groups": []` back, which removes the network access; a reinstalled older click would hit the same DNS failure. The user's data is untouched: tasks rows created by the new code are ordinary `failed` rows, and the old code reads them fine. Before leaving draft, per docs/09 "On the phone": install the dev click on Plinius and check that GerSch downloads. This is the one thing the CI cannot prove. Also check Cancel and Cancel all on the progress view. Owner phone try-list: - an unreachable source fails one item and the rest install - cancel a queued, a running and a failed item - retry a failed item - kill mid-download and reopen - install a Bible after all that ## Proof - Gates, green on head 60d11c7 (later commits 6347524 and c6ef183 added the keep-bytes fix, wake-after-send and an e2e tweak; `cargo make` was green locally before the push, and CI ran app and e2e on the pushed head). - `cargo make`: tests 69+8+3+3+170+15+4 passed, 0 failed; coverage TOTAL lines 94.80% (regions 94.50%); deny and audit ok. - `cargo make app-docker`: EXIT 0. "translations: po/ is up to date with the sources", "utsb: 70 translated messages". mem-check normal: PSS 151421 kB (budget 165000 kB), slow: 151969 kB. - `cargo make e2e-docker`: "e2e: 21 passed, 0 failed, 0 skipped" and, slow profile, "e2e: 4 passed, 0 failed, 0 skipped". New: PASS install-queue-fail (23s): BSB answers 503 and is marked failed ("the server answered HTTP 503"), TSK installs, then Retry on the row installs BSB. PASS install-queue-cancel (29s): a queued row is cancelled and never fetched, Cancel all stops the running download, the tasks dir is empty, the choice is back and an install works. - FAIL without the change, e2e against origin/main with the new case copied in: `FAIL install-queue-fail (100s)`, `install-queue-fail: FAIL: no 'utsb: install BSB failed' (x1) after 90 s`, `e2e: 0 passed, 1 failed`. On base the 503 source is retried for ever and TSK never installs. - Unit tests, each run with a deliberate break (change broken, test failing): - No retry limit, sword: `test install::http::tests::a_source_that_keeps_failing_ends_the_download ... FAILED` - No retry limit, core: `test tasks::tests::a_source_that_keeps_failing_fails_its_task_and_the_next_one_installs ... FAILED` (GerSch retries 3 times, then installs "Done", so no failure is ever reported) - No wake: `test tasks::tests::a_cancel_does_not_wait_for_the_backoff ... FAILED` - CancelAll a no-op: `cancel_all_also_forgets_a_failed_item ... FAILED` and `cancel_all_clears_the_queue_the_running_download_and_the_failed ... FAILED` - Cancel keeps failed rows: `a_failed_item_can_be_cancelled_and_stays_gone ... FAILED` and `cancel_all_also_forgets_a_failed_item ... FAILED` - No orphan sweep: `files_left_for_a_task_that_is_gone_are_swept_at_start ... FAILED` - Board cancel keeps the row: `tasks::progress::tests::a_cancelled_row_goes_and_late_events_for_it_change_nothing ... FAILED` - AppArmor with `"policy_groups": []`, i.e. the origin/main file: `test tasks::tests::the_click_may_use_the_network ... FAILED` - Passing-only tests that guard existing behaviour: `bytes_arriving_start_the_count_of_failures_again`, `a_wake_during_a_retry_wait_is_reported`. - New tests: `a_download_that_gave_up_keeps_its_bytes_for_the_retry` (a retry after give-up asks for the Range from the kept length and installs), `a_failed_item_dismissed_takes_its_bytes`, `a_stopped_download_s_late_word_does_not_start_a_new_row`. Two existing board tests were adjusted to send Started first. - PSS, measured by mem-check (CI amd64, Xvfb, not the phone). Base origin/main: normal 149641 kB, slow 151135 kB. This branch: normal 151421 kB, slow 151969 kB; an earlier run of the same code gave 154256 and 155676 kB. The spread between runs (about 4 MB) is larger than the difference to base, so no measurable effect; the 165000 kB budget holds. Added resident state is one Arc<AtomicBool> and a few enum arms. The two row buttons exist only while the progress page is open and rows are listed. Start-up work is unchanged, apart from one read_dir of the tasks folder (empty in the normal case) for the orphan sweep. - Deviation: one write on the phone. I copied app.sqlite3* to the phone's /tmp to read it off, then removed those copies. Nothing else was touched; the app was not killed or launched. - I did not run app-docker or e2e-docker locally, because nothing in qt/ or QML changed in the last commits; CI ran both and they passed. ## Review **Reviewer:** fresh-context `code-review`, review #257 on this PR, with responses in the thread; open findings: none. **Review-Confidence:** 70/100, copied unadjusted from the audit, with its deductions (the AppArmor effect cannot be exercised off the device). ## Confidence by part - Retry limit, failure handling, cancel and retry in `sword/src/install` and `core/src/tasks`: high. Each behaviour has a unit test that fails when the change is broken, plus two e2e cases. - QML buttons and qt/ glue: medium. Covered by e2e, not tried on the phone. - AppArmor `networking` group: medium. The diagnosis rests on the profile contents and the symptom, with no DENIED line; only the phone check proves it. A human should read this part and try it on the device. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Install queue: failures never block, cancel and retry (#74)
Some checks failed
CI / rust (pull_request) Has been cancelled
CI / audit (pull_request) Has been cancelled
CI / app (pull_request) Has been cancelled
CI / e2e (pull_request) Has been cancelled
CI / click (pull_request) Has been cancelled
Presets / preset-check (pull_request) Has been cancelled
60d11c7e07
Root cause of the GerSch failure: the click's AppArmor policy had no
policy groups, so the confined app could not use the network (no
nameservice, no sockets) and every name lookup failed with "temporary
failure in name resolution" while the shell resolved fine. Add the
networking group.

Queue: a download gives up after Options::max_retries failed requests in
a row (bytes arriving reset the count), so the task is marked failed and
the next one starts. Cancel for queued, running and failed items, and
Cancel all; a cancel deletes the row first, then the partial files, and
files left by a kill are swept at the next start. A waiting backoff is
woken by a command, so a cancel is prompt. Retry per row. e2e cases for
a failing source and for cancel.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review requested changes 2026-10-23 15:11:58 +00:00
Dismissed
claude-review left a comment

Level: high. Commit: 60d11c7. Range: origin/main...HEAD. Blast radius: trunk (core/src/tasks, sword/src/install, qt glue).

Files: core/src/db/tasks.rs, core/src/db/tasks/tests.rs, core/src/tasks.rs, core/src/tasks/exec.rs, core/src/tasks/progress.rs, core/src/tasks/tests.rs, docs/04-features.md, po/de.po, po/utstudybible.schaefera.pot, qml/Main.qml, qml/pages/StarterPage.qml, qt/src/installer.rs, sword/src/install.rs, sword/src/install/http.rs, sword/src/install/http/tests.rs, tools/e2e/cases/install-queue-cancel.sh, tools/e2e/cases/install-queue-fail.sh, tools/e2e/lib.sh, tools/e2e/repo-server.py, utstudybible.apparmor

Checked and fine: a failed row no longer blocks the queue (next_task takes only queued rows). Cancel of a running download works, including during a backoff, because the wake flag lets the loop look at commands. The stop flag is reset after a cancel unless paused. The row goes before the files, and sweep_orphans cleans leftovers after a kill. The apparmor "networking" policy group is the plausible DNS root cause. Without it the confined app cannot resolve names, and the backoff cap only keeps that from blocking forever.

Findings:

  1. MEDIUM sword/src/install/http.rs:306 with core/src/tasks/exec.rs finish(): after 5 failed requests in a row (~15 s offline) the download returns Err. Outcome::Failed then calls clean(), which deletes .part and .state. Scenario: a 90% downloaded module on a phone that drops signal for 15 s (tunnel, lift, a Wi-Fi to cellular switch) restarts from 0 on Retry. Before this PR a network error retried forever and kept the bytes, and the app is meant to work on low-end phones with poor links. Fix: keep the partial files on a failure whose cause is network or 5xx (clean only on Done or Cancelled), or raise the budget or make it time-based (e.g. ~2 min). Update the e2e assertion "the failed download kept its bytes" to match.
  2. LOW core/src/tasks.rs:200: Runner::send sets the wake flag before tx.send. The tasks thread can swap the flag, emit Event::Wake and try_recv before the command is in the channel. The command is then not seen until the next progress event or the end of the backoff (up to 30 s). A cancel in that window is delayed, and the UI row is already gone. Fix: send first, then set the flag.
  3. LOW core/src/tasks/exec.rs install(): commands are only handled in on_download. A Cancel during package::install is processed after the install finishes. The module ends up installed (the mods.d conf is written) although the user cancelled, and the board row is gone with no event. Either document this or make Cancel after the download remove the module.
  4. LOW qt/src/installer.rs cancel_row then retry_row (or a new install) of the same module straight away: late Progress events from the still-stopping old download are applied to the new board row. Cosmetic only.

No findings outside the diff beyond the above.

Level: high. Commit: 60d11c7. Range: origin/main...HEAD. Blast radius: trunk (core/src/tasks, sword/src/install, qt glue). Files: core/src/db/tasks.rs, core/src/db/tasks/tests.rs, core/src/tasks.rs, core/src/tasks/exec.rs, core/src/tasks/progress.rs, core/src/tasks/tests.rs, docs/04-features.md, po/de.po, po/utstudybible.schaefera.pot, qml/Main.qml, qml/pages/StarterPage.qml, qt/src/installer.rs, sword/src/install.rs, sword/src/install/http.rs, sword/src/install/http/tests.rs, tools/e2e/cases/install-queue-cancel.sh, tools/e2e/cases/install-queue-fail.sh, tools/e2e/lib.sh, tools/e2e/repo-server.py, utstudybible.apparmor Checked and fine: a failed row no longer blocks the queue (next_task takes only queued rows). Cancel of a running download works, including during a backoff, because the wake flag lets the loop look at commands. The stop flag is reset after a cancel unless paused. The row goes before the files, and sweep_orphans cleans leftovers after a kill. The apparmor "networking" policy group is the plausible DNS root cause. Without it the confined app cannot resolve names, and the backoff cap only keeps that from blocking forever. Findings: 1. MEDIUM sword/src/install/http.rs:306 with core/src/tasks/exec.rs finish(): after 5 failed requests in a row (~15 s offline) the download returns Err. Outcome::Failed then calls clean(), which deletes .part and .state. Scenario: a 90% downloaded module on a phone that drops signal for 15 s (tunnel, lift, a Wi-Fi to cellular switch) restarts from 0 on Retry. Before this PR a network error retried forever and kept the bytes, and the app is meant to work on low-end phones with poor links. Fix: keep the partial files on a failure whose cause is network or 5xx (clean only on Done or Cancelled), or raise the budget or make it time-based (e.g. ~2 min). Update the e2e assertion "the failed download kept its bytes" to match. 2. LOW core/src/tasks.rs:200: Runner::send sets the wake flag before tx.send. The tasks thread can swap the flag, emit Event::Wake and try_recv before the command is in the channel. The command is then not seen until the next progress event or the end of the backoff (up to 30 s). A cancel in that window is delayed, and the UI row is already gone. Fix: send first, then set the flag. 3. LOW core/src/tasks/exec.rs install(): commands are only handled in on_download. A Cancel during package::install is processed after the install finishes. The module ends up installed (the mods.d conf is written) although the user cancelled, and the board row is gone with no event. Either document this or make Cancel after the download remove the module. 4. LOW qt/src/installer.rs cancel_row then retry_row (or a new install) of the same module straight away: late Progress events from the still-stopping old download are applied to the new board row. Cosmetic only. No findings outside the diff beyond the above.
@ -192,3 +197,4 @@
/// Queues a command and returns its number (increasing from 1).
pub fn send(&mut self, command: Command) -> u64 {
self.next += 1;
self.wake.store(true, Ordering::Relaxed);
Collaborator

LOW: the wake flag is set before the command is sent, so the tasks thread can swap the flag and find an empty channel. The command then waits for the next progress event or the end of the backoff (up to 30 s). Send first, then set the flag.

LOW: the wake flag is set before the command is sent, so the tasks thread can swap the flag and find an empty channel. The command then waits for the next progress event or the end of the backoff (up to 30 s). Send first, then set the flag.
@ -287,2 +303,4 @@
// Whatever arrived before the error is kept.
self.off = self.part.metadata()?.len();
failures = if self.off > before { 1 } else { failures + 1 };
if failures >= self.c.opts.max_retries {
Collaborator

MEDIUM: giving up after 5 failures (~15 s) ends in Outcome::Failed, whose clean() deletes .part and .state. A 90% done download on a phone that loses signal for 15 s restarts from 0 on Retry. Keep the partial files for network and 5xx failures, or allow a longer budget.

MEDIUM: giving up after 5 failures (~15 s) ends in Outcome::Failed, whose clean() deletes .part and .state. A 90% done download on a phone that loses signal for 15 s restarts from 0 on Retry. Keep the partial files for network and 5xx failures, or allow a longer budget.
Install queue: a failed download keeps its bytes; wake after send; a cancel during unpacking removes the module; late progress ignored on queued rows (#74)
Some checks failed
CI / rust (pull_request) Has been cancelled
CI / audit (pull_request) Has been cancelled
CI / app (pull_request) Has been cancelled
CI / e2e (pull_request) Has been cancelled
CI / click (pull_request) Has been cancelled
Presets / preset-check (pull_request) Has been cancelled
6347524e66
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e2e: a failed download keeps its (here no) bytes; the unit tests cover the rest (#74)
All checks were successful
CI / rust (pull_request) Successful in 7m19s
Presets / preset-check (pull_request) Successful in 3m45s
CI / audit (pull_request) Successful in 17s
CI / app (pull_request) Successful in 3m57s
CI / e2e (pull_request) Successful in 7m3s
CI / click (pull_request) Successful in 3m15s
c6ef1834d9
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
Collaborator

Review 257 answers (claude-bot), fixes in 6347524 and c6ef183:

  1. MEDIUM, bytes lost on give-up: fixed. A download that gives up on Error::Network or a 5xx now ends as Outcome::Interrupted: the row is Failed but .part/.state stay, so Retry resumes by Range. Other failures (4xx, bad module) and Cancel/Dismiss still clean. Tests: a_download_that_gave_up_keeps_its_bytes_for_the_retry (retry asks from the kept length) and a_failed_item_dismissed_takes_its_bytes. The e2e assertion that the .part is gone was removed.
  2. LOW, wake before send: fixed. Runner::send now sends, then sets the flag. The race is timing-only, so no deterministic test; a_cancel_does_not_wait_for_the_backoff still passes.
  3. LOW, cancel during package::install: fixed. Commands are drained before and after package::install; a cancel that arrived removes the module again and ends as Cancelled. No test: no event fires inside that window, so it cannot be injected deterministically.
  4. LOW, late Progress on a new row: fixed in Board. Progress and Retrying are ignored while a row is Queued (only Started moves it on). Test: a_stopped_download_s_late_word_does_not_start_a_new_row.
Review 257 answers (claude-bot), fixes in 6347524 and c6ef183: 1. MEDIUM, bytes lost on give-up: fixed. A download that gives up on Error::Network or a 5xx now ends as Outcome::Interrupted: the row is Failed but .part/.state stay, so Retry resumes by Range. Other failures (4xx, bad module) and Cancel/Dismiss still clean. Tests: `a_download_that_gave_up_keeps_its_bytes_for_the_retry` (retry asks from the kept length) and `a_failed_item_dismissed_takes_its_bytes`. The e2e assertion that the .part is gone was removed. 2. LOW, wake before send: fixed. Runner::send now sends, then sets the flag. The race is timing-only, so no deterministic test; `a_cancel_does_not_wait_for_the_backoff` still passes. 3. LOW, cancel during package::install: fixed. Commands are drained before and after package::install; a cancel that arrived removes the module again and ends as Cancelled. No test: no event fires inside that window, so it cannot be injected deterministically. 4. LOW, late Progress on a new row: fixed in Board. Progress and Retrying are ignored while a row is Queued (only Started moves it on). Test: `a_stopped_download_s_late_word_does_not_start_a_new_row`.
claude-review left a comment

Level: medium. Commit: c6ef1834d9. Range: 60d11c7..c6ef183 (re-review of the fixes to review 257).

Files: core/src/tasks.rs, core/src/tasks/exec.rs, core/src/tasks/progress.rs, core/src/tasks/tests.rs, tools/e2e/cases/install-queue-fail.sh

Findings: none high or medium. I checked each of these against the code and found no defect.

  • A failed or cancelled item cannot block the queue. next_task selects only queued rows, so a failed row is skipped.
  • A cancel during the download, after the last progress or during the unpack ends as Cancelled: the files are cleaned, or the module is removed after the unpack.
  • A kept .part is safe. It stays only for Network and 5xx failures, it is tied to the .state validator, and sweep_orphans leaves it while its task row exists. Cancel or dismiss deletes it.
  • The wake flag is now set after the send, so a woken runner always finds the command.
  • A late Progress or Retrying event on a Queued row is ignored, so it can no longer revive a cancelled row.
  • A DNS failure ("temporary failure in name resolution") now fails the task as "no connection". The failed item no longer blocks the next one, and the user can Retry or Cancel it.

Low, outside the diff (not blocking): a cancel that arrives during package::install is only handled after the unpack finishes, so the module is installed and then removed. This is correct but wastes work on a slow phone. Failed items with kept bytes stay on disk until the user retries or dismisses them.

Level: medium. Commit: c6ef1834d9b186b80d4118d8f900323f25a7e285. Range: 60d11c7..c6ef183 (re-review of the fixes to review 257). Files: core/src/tasks.rs, core/src/tasks/exec.rs, core/src/tasks/progress.rs, core/src/tasks/tests.rs, tools/e2e/cases/install-queue-fail.sh Findings: none high or medium. I checked each of these against the code and found no defect. - A failed or cancelled item cannot block the queue. next_task selects only queued rows, so a failed row is skipped. - A cancel during the download, after the last progress or during the unpack ends as Cancelled: the files are cleaned, or the module is removed after the unpack. - A kept .part is safe. It stays only for Network and 5xx failures, it is tied to the .state validator, and sweep_orphans leaves it while its task row exists. Cancel or dismiss deletes it. - The wake flag is now set after the send, so a woken runner always finds the command. - A late Progress or Retrying event on a Queued row is ignored, so it can no longer revive a cancelled row. - A DNS failure ("temporary failure in name resolution") now fails the task as "no connection". The failed item no longer blocks the next one, and the user can Retry or Cancel it. Low, outside the diff (not blocking): a cancel that arrives during package::install is only handled after the unpack finishes, so the module is installed and then removed. This is correct but wastes work on a slow phone. Failed items with kept bytes stay on disk until the user retries or dismisses them.
claude-bot changed title from WIP: Install queue: failures never block, cancel and retry (#74) to Install queue: failures never block, cancel and retry (#74) 2026-10-23 15:43:41 +00:00
claude-review left a comment

Gate approval on c6ef183 (trunk). The one medium finding (bytes lost after the retry limit) is fixed in 6347524. Network errors and 5xx now end as Outcome::Interrupted, which keeps .part and .state so Retry resumes by Range. 4xx, bad-module failures, Cancel and Dismiss still clean. Unit tests cover both paths: a_download_that_gave_up_keeps_its_bytes_for_the_retry and a_failed_item_dismissed_takes_its_bytes. The three low findings are fixed too: wake is set after send, commands are drained around package::install with the module removed on a late cancel, and the Board ignores late Progress and Retrying on a Queued row (tested). I read both commits after review 257 (6347524, c6ef183). The only change in c6ef183 is dropping an e2e assertion that the new behaviour makes wrong. CI is green on head: rust, audit, app, e2e, click and preset-check. The audit scored 70 and missed no defects. The PR body's Proof lists no unrun elevated or maximum gate. The only open item is the owner's phone trial of the AppArmor networking group, which is listed as a try-list and is the owner's call.

Gate approval on c6ef183 (trunk). The one medium finding (bytes lost after the retry limit) is fixed in 6347524. Network errors and 5xx now end as Outcome::Interrupted, which keeps .part and .state so Retry resumes by Range. 4xx, bad-module failures, Cancel and Dismiss still clean. Unit tests cover both paths: a_download_that_gave_up_keeps_its_bytes_for_the_retry and a_failed_item_dismissed_takes_its_bytes. The three low findings are fixed too: wake is set after send, commands are drained around package::install with the module removed on a late cancel, and the Board ignores late Progress and Retrying on a Queued row (tested). I read both commits after review 257 (6347524, c6ef183). The only change in c6ef183 is dropping an e2e assertion that the new behaviour makes wrong. CI is green on head: rust, audit, app, e2e, click and preset-check. The audit scored 70 and missed no defects. The PR body's Proof lists no unrun elevated or maximum gate. The only open item is the owner's phone trial of the AppArmor networking group, which is listed as a try-list and is the owner's call.
claude-bot deleted branch fix/install-queue 2026-10-23 15:46:59 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 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
schaefera/utStudyBIble!76
No description provided.