Install queue: failures never block, cancel and retry (#74) #76
No reviewers
Labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
schaefera/utStudyBIble!76
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/install-queue"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.apparmorhad"policy_groups": [](since the repo-setup commit9f9beba), but D10 and docs/07 say the app needsnetworking. It now lists it.Second bug, the softlock itself:
Transfer::runretried 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.1has nonetworkrule 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>.install GerSch downloadingthenretryingin 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.runningand the other fivequeued; the tasks dir has an empty GerSch.zip.part.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 ordinaryfailedrows, 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:
Proof
60d11c7(later commits6347524andc6ef183added the keep-bytes fix, wake-after-send and an e2e tweak;cargo makewas 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 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.test install::http::tests::a_source_that_keeps_failing_ends_the_download ... FAILEDtest 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)test tasks::tests::a_cancel_does_not_wait_for_the_backoff ... FAILEDcancel_all_also_forgets_a_failed_item ... FAILEDandcancel_all_clears_the_queue_the_running_download_and_the_failed ... FAILEDa_failed_item_can_be_cancelled_and_stays_gone ... FAILEDandcancel_all_also_forgets_a_failed_item ... FAILEDfiles_left_for_a_task_that_is_gone_are_swept_at_start ... FAILEDtasks::progress::tests::a_cancelled_row_goes_and_late_events_for_it_change_nothing ... FAILED"policy_groups": [], i.e. the origin/main file:test tasks::tests::the_click_may_use_the_network ... FAILEDbytes_arriving_start_the_count_of_failures_again,a_wake_during_a_retry_wait_is_reported.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.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
sword/src/installandcore/src/tasks: high. Each behaviour has a unit test that fails when the change is broken, plus two e2e cases.networkinggroup: 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
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:
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);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 {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.
Review 257 answers (claude-bot), fixes in
6347524andc6ef183:a_download_that_gave_up_keeps_its_bytes_for_the_retry(retry asks from the kept length) anda_failed_item_dismissed_takes_its_bytes. The e2e assertion that the .part is gone was removed.a_cancel_does_not_wait_for_the_backoffstill passes.a_stopped_download_s_late_word_does_not_start_a_new_row.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.
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.
WIP: Install queue: failures never block, cancel and retry (#74)to Install queue: failures never block, cancel and retry (#74)Gate approval on
c6ef183(trunk). The one medium finding (bytes lost after the retry limit) is fixed in6347524. 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 inc6ef183is 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.