F1 starter sets 1/2: presets, resumable install tasks, preset CI check #26
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!26
Loading…
Reference in a new issue
No description provided.
Delete branch "phase1/starter-sets-core"
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
The core half of F1 starter sets, with no UI yet. It adds five starter sets (
presets/*.toml: English, Deutsch, Français, original languages, study) that state each module's licence and sizes. It adds a SWORD installer insword/(CrossWire index, resumable rawzip downloads, install and remove), and a resumable task queue incore/: migration 002 and a runner thread that survives kill -9 mid-download. A weekly CI job checks the sets against CrossWire's live index. Part of #10 (part 1/2; part 2 is the module manager and first-run picker UI).Blast radius
Tier: trunk
Trunk paths touched:
sword/src/install*,core/src/db/(002_tasks.sql, tasks.rs, db.rs),core/src/tasks/,presets/,Cargo.toml,deny.toml,Makefile.toml,.forgejo/workflows/presets.yml,tools/ci/preset-check/,qt/Cargo.lockTesting tier: maximum (new dependencies: ureq/rustls, zip 9, toml, serde; a new CI workflow). Also elevated: downloads from the network, migration 002.
Gate / rollback: nothing is wired into the app yet, because the Qt glue does not start the runner or installer. Rollback is a revert of this merge. Migration 002 only adds the
taskstable, andapp.sqlite3is backed up (VACUUM INTO) before it runs. The previous build would see schema version 2, so rolling back the app needs that.bak-v1.Proof
cargo make app-dockermem-check in CI's clickable image, run locally. This is the preview.cargo-denyandcargo-auditpass on the new dependencies, which include zip 9 rather than 2.x because of RUSTSEC advisories.c18f37e, and exit 1 against a deliberately broken copy of the presets.https://crosswire.org/ftpmirror/pub/sword. Only GET and HEAD reads, no writes. Installs run only against the local test server.concurrent_opens_of_an_old_file_all_migrate), and mem-check's freshapp.sqlite3. The app migrated, started and restored state on3b616f1. No user database was touched.cargo makeexit 0 onc18f37e: fmt, clippy, tests with coverage (core lib 98 passed), release check, cargo-denyadvisories ok, bans ok, licenses ok, sources ok, cargo-audit clean (root and qt).cargo make app-dockerexit 0 on the merged tree3b616f1: start-up 134 ms, peak PSS 147478 kB (budget 165000), kill -9 restore ok, picker checks ok.sword/src/install/testserver.rs) that cuts connections, fails with statuses, changes files mid-download and ignores ranges. None of them touches the network.concurrent_opens_of_an_old_file_all_migrate.cargo run --release -p utsb-preset-checkgivesok: 5 sets, 23 modules match https://crosswire.org/ftpmirror/pub/sword. A deliberately broken copy exits 1 with 3 problems.3b616f1reportedStrongsGreek: no rawzip ... Peer disconnected): a HEAD that gets no answer is retried 4 times. If it never answers, the job exits 2 ("could not check") and the remaining modules are not asked. Tests count the HEAD calls (2/4/1), and each fails without its change.c18f37e, the last code commit.include_str!(about 9 kB).check_live) but not at CLI level, because the test server cannot fail a HEAD at the network level.Review
Reviewer: fresh-context review at high (
7950ada): 8 findings (2 medium, 6 low), all 8 fixed in8bd9a64. Re-review at medium: 1 low (issue dedupe), fixed in9aa196a. Coverage review at medium (209): 9 lows, 5 fixed ince21b29, 1 deferred to #28 (no 416 path), 3 declined as by design: the test server has no Drop (harmless), presets.yml runs on PRs against live CrossWire (intended), and the winnow duplicate is a deny warning only. Delta review 213: 1 low (exactRawGenBookmatch), declined, because ModDrv values are fixed tokens. Delta review 214: 4 lows, 2 fixed inc18f37e, 2 deferred to #29. Delta review 215: no findings.Review-Confidence: 55/100. A trunk-tier high review plus four delta reviews cover every code change, CI is green, and the auditor's own read of
http.rsfound no missed defect. Points off for the 416 resume gap and the preset-check error-handling notes (now #28, #29), and because four of the fivepresets/*.tomlfiles were never listed as reviewed (the live check backs their contents).Confidence by part
sword/src/install/http.rs(resumable download): high. Every resume path has a mutant-killing test against the cutting test server. Read it for the 416 gap.sword/src/install/package.rs(rawzip install/remove, ustar reader): high. DataPath confinement, the byte budget and the conf-last order are each tested.core/src/db.rs+ 002 (migration lock): medium-high. The concurrent-open test covers the race, but the process-wide lock is new.core/src/tasks/(runner): medium. Two cancel paths are proven only by a timeout, and the runner is not wired into the app yet.presets/*.tomlsizes and licences: medium. They match CrossWire today, and the weekly job watches them..forgejo/workflows/presets.ymlissue step: low. It was proven against a mock API only. Read it.Follow-ups for part 2: wire the runner into qt/, add an index-fetch command, decide on an auto-retry policy, measure click size and PSS, and add assertion tests for the two hang-only mutants.
🤖 Generated with Claude Code
Fresh-context review (posted by the author account)
Reviewed
7950adaf. Two medium and six low findings, inline below. Each gets an answer in its thread after the fix.@ -0,0 +40,4 @@/opt/rust/cargo/registry/cachekey: presets-deps-${{ hashFiles('Cargo.lock') }}restore-keys: |presets-deps-[low] The weekly schedule job fails silently when CrossWire changes a module. Nobody is notified unless the Actions page is opened, so a stale size or licence shown in the picker can persist for months. The docs describe the schedule as the safeguard: add a notification or open an issue on failure, or document where the result is read.
@ -113,6 +116,9 @@ impl Db {fn try_open(path: &Path, steps: &[&str]) -> Result<Db> {let mut conn = Connection::open(path)?;// The task runner has its own connection: wait for a write lock[medium] The task runner and the reading worker open the same app.sqlite3. On the first launch after upgrading from schema 1, both read user_version=1 before either takes a write lock, both run the backup (VACUUM INTO deletes and recreates the same .bak-v1), and both run 002_tasks.sql. The loser fails with 'table tasks already exists' (or SQLITE_BUSY_SNAPSHOT), which is not is_corrupt, so Db::open returns Err. If the runner loses, it reports Event::Error and the thread ends: later sends are silently dropped and no install runs. If the reader loses, the app falls back to the in-memory DB and loses saved state. busy_timeout does not help because the version check is outside the transaction. Fix: BEGIN IMMEDIATE and re-read user_version inside it (or serialise the open); add a test that opens twice concurrently on a v1 file.
@ -0,0 +173,4 @@let _ = t.join();}}}[low] If Db::open fails, run() emits Event::Error(0) and returns. The Runner stays alive with its receiver dropped, so send() silently discards every command and pause/resume do nothing. The picker would show a queued install that never starts, with no further error. Keep the thread alive and answer each command with an error, or expose a 'dead' state.
@ -0,0 +125,4 @@if let Err(e) = self.db.start_task(task.id) {return self.emit(Event::Error(format!("tasks: {e}")));}self.current = Some((module.clone(), false));[medium] If db.start_task fails (locked past the 5 s busy timeout, read-only, full disk), execute emits Event::Error and returns with the row still 'queued'. The run loop calls next_task, which returns the same row, and execute runs again: a hot loop emitting an Error per iteration, burning battery. Unlike the next_task error path it does not set paused. The same applies when a later fail_task/finish_task keeps failing. Pause the runner on this error, as next_task does.
@ -0,0 +205,4 @@/// A download event: progress goes to the table and the listener, and/// commands that arrived meanwhile are handled (a cancel stops it).fn on_download(&mut self, rx: &Receiver<(u64, Command)>, task: &Task, ev: &install::Event) {let progress = match *ev {[low] A Cancel drained in on_download during the final Event::Downloaded progress does not stop the download, which is already complete. install() goes on to package::install and installs the module. execute then overrides the outcome with Outcome::Cancelled, so the row is gone and no Done or Failed event is emitted, but the module is installed on disk and shows in the Bible list. Check the flag before package::install, or treat Done as authoritative.
@ -0,0 +45,4 @@let agent = Agent::config_builder().timeout_connect(Some(Duration::from_secs(15))).timeout_recv_response(Some(Duration::from_secs(30))).timeout_recv_body(Some(opts.chunk_timeout))[low] timeout_recv_body is a total budget for the body (60 s by default), sized for a 512 KiB chunk. When a mirror answers a Range or If-Range request with 200 (CrossWire's ftpmirror redirects to mirrors whose ETag and Last-Modified differ), accept_whole restarts and reads the whole file under that same budget. A rawzip of a few tens of MB on a slow mobile link times out, becomes Step::Retry, and the next attempt may get 200 again and restart from 0. The download never completes. Give the whole-file path a longer or progress-based timeout, or cap the restarts.
@ -0,0 +62,4 @@for i in 0..zip.len() {let mut entry = zip.by_index(i).map_err(|e| bad(e.to_string()))?;let rel = entry.enclosed_name().ok_or_else(|| bad(format!("unsafe path {:?}", entry.name())))?;if entry.is_dir() {[low] unpack copies each zip entry with io::copy and no limit on decompressed size, total size or entry count. A zip bomb (a mirror compromise or a bad package) of a few KB can fill the phone's flash; the ENOSPC can break other app writes such as save-state before the staging folder is cleaned. Bound the total extracted bytes and the entry count.
@ -0,0 +207,4 @@Ok(DataPath::Prefix { dir: parts.iter().collect(), stem })}/// Whether `file` (relative to the SWORD folder) is under this path.[low] A conf whose DataPath has no trailing slash and exactly four components (for example ./modules/texts/ztext/kjv) parses as Prefix{dir: modules/texts/ztext, stem: kjv}. owns() then rejects the module's real files under ztext/kjv/ as outside the DataPath, and remove_data would call fs::remove_file on a directory in the shared driver folder and fail after the conf is already deleted, leaving the data orphaned. Treat a text-driver DataPath as Dir.
Answers to the fresh-context review of
7950adaf, fixed in8bd9a64b. Gates:cargo makegreen (fmt, clippy, tests with coverage, release, deny, audit),cargo make presetsok (5 sets, 23 modules).concurrent_opens_of_an_old_file_all_migrate(four threads on a v1 file), failed first. Fix: a process-wide lock around version check, backup and migrate, plus each step under BEGIN IMMEDIATE with user_version re-read (covers another process). Passes.a_database_that_refuses_the_start_pauses_the_queue_instead_of_spinning(trigger aborting UPDATE on tasks) hung on the old code. Fix: a refused start_task, or a failed finish/fail/requeue, halts the queue like the next_task path; one Error, then Paused.a_whole_file_gets_a_budget_for_its_sizeand the updated no-ranges test.a_cancel_that_arrives_with_the_last_progress_installs_nothingfails on the old code. install() now returns Cancelled before package::install if the flag is set.a_queue_that_cannot_open_its_database_answers_every_command).a_package_that_unpacks_past_the_limits_is_refused).a_text_module_whose_data_path_has_no_trailing_slash_installs_and_removes.Fresh-context review (posted by the author account)
Reviewed
8bd9a64. One low finding, inline below.@ -0,0 +64,4 @@if echo "$open" | grep -q "\"title\":\"$title\""; thenid=$(echo "$open" | grep -o '"number":[0-9]*' | head -1 | cut -d: -f2)body="{\"body\":\"Still failing: $RUN\"}"curl -fsS -X POST -H "Authorization: token $TOKEN" -H "Content-Type: application/json" -d "$body" "$API/issues/$id/comments" || truelow: the dedupe step takes the first "number": from the search response, not the one belonging to the matching title. If q=Starter-set+presets also returns another open issue, grep -q matches the right title but head -1 picks the other issue's number, so the 'Still failing' comment lands on the wrong issue. Failures hidden by
|| trueare accepted (the comment says so).Answer to the presets.yml:67 finding: fixed in
9aa196a. Confirmed real (old logic returned 7 for a two-issue response where the match was 12). The step now pairs each "number" with the next "title" and takes the number of the title that matches; checked against a sample response (returns 12) and the YAML still parses. cargo make passes.Coverage review (medium)
Range: origin/main...origin/phase1/starter-sets-core at
9aa196a, plus the commit range 8bd9a64..9aa196a (presets.yml issue-dedupe fix).Files reviewed:
(read for context: sword/src/install/{http,index,package}.rs, sword/src/install.rs, core/src/tasks.rs, core/src/tasks/exec.rs, core/src/db/tasks.rs, 002_tasks.sql, sword/Cargo.toml, core/Cargo.toml)
Result: no high or medium findings. The tests passed 3 runs in a row under parallel cargo test (sword install: 36 tests; core tasks and db::tasks: 26 tests), touch only 127.0.0.1, and
cargo deny --offline check licenses bansis ok. The new crates (ureq 3.5, rustls 0.23, ring, rustls-webpki, webpki-roots, zip 9, toml 0.9, serde, typed-path, zeroize, subtle) are all MIT, Apache-2.0, ISC or BSD, plus CDLA-Permissive-2.0 for webpki-roots only (the crate-scoped exception is correct). None is GPL-2.0-only. The presets.yml dedupe fix is correct: "number" is 9 characters so substr($0,10) is right, and the issue's own title comes before the label and milestone titles in Forgejo's JSON.Low findings:
low · core/src/tasks/tests.rs:373-390 · a_cancel_that_arrives_with_the_last_progress_installs_nothing does not reach the branch it names. The Cancel is already queued, so Ctx::on_download reads it on the first download event, which is Event::Start, not the last progress. The stop flag is then set before any byte is fetched, download returns Err(Cancelled) and the task ends as Cancelled through exec.rs:459. The guard at core/src/tasks/exec.rs:462 (Ok(()) with cancelled set) is never executed. Deleting that guard keeps every test green, and a real cancel that lands on the final Progress or Downloaded event would then install the module and report Done after the user cancelled. Fix: send the Cancel from inside the event callback only when the event is Downloaded (or the final Progress).
low · core/src/tasks/exec.rs:409-411 · The Done branch of finish() emits Event::Done before db.finish_task, while the Failed branch (exec.rs:415-416) writes the row first. A listener that reloads the task list on Done can read the row as still 'running'. The tests only look at the table after Idle, so they cannot see it. Fix: write the row first in the Done branch, and add a test that reads db.tasks() inside the callback on Done.
low · sword/src/install/package.rs:213-219 · DataPath::for_conf treats every driver except RawLD/zLD/RawLD4 as a folder. SWORD's RawGenBook keeps DataPath as a file-name prefix (./modules/genbook/rawgenbook/x/x with x.bdt, x.bdx, ...). For a genbook, check() would refuse the install with "outside the module's DataPath". The tests cover only zText and a prefix-style lexicon. No current preset has a genbook, so nothing breaks today, but the module manager will offer any module in the index.
low · sword/src/install/testserver.rs:127-131 and 135-169 · The test server differs from real servers in two ways that hide client behaviour. (a) It answers a Range that starts past the end with 200 and the whole file, where Apache answers 416 (parse_range returns None when a > b). The client's 416 path (http.rs:338, Fatal Http(416)) is never exercised. (b) It never redirects. CrossWire's /ftpmirror/ URL redirects to mirrors, ureq follows the redirect and re-sends Range and If-Range to a host with a different ETag or Last-Modified. The result is a 200 and a restart (http.rs:372-392) that no test models. Fix: add a redirect knob to the server and a test that resumes against a second origin with another ETag.
low · sword/src/install/http/tests.rs:90-102 (target: http.rs:137, 361-375, 218-223) · Three resume branches have no test. (a) The Last-Modified fallback of State::validator, because the server never sends Last-Modified or weak ETags. (b) accept_range's "size changed" restart, the "server ignores If-Range" case, because the server always honours If-Range, so a_changed_file_restarts only covers the 200 path. (c) The "part longer than total" restart at http.rs:137. A regression there would write mixed-version bytes into the zip, and only the zip CRC would catch it.
low · sword/src/install/package/tests.rs:451-455 · a_package_that_unpacks_past_the_limits_is_refused does not prove the budget is cumulative. CONF is 51 bytes, so with bytes: 50 the first entry already trips the limit. Code that checked each entry against the limit separately would pass. Fix: use two entries of 30 bytes with bytes: 50.
low · sword/src/install/testserver.rs:46-53 · Server has no Drop. Each Server::start leaves its accept thread blocked and the listening socket open until the test process exits. Pooled keep-alive connection threads also live until the client agent closes. With about 60 tests it is harmless, but the preset check and core tests will keep adding to it. Fix: add a Drop that sets a flag and connects once to wake the accept loop.
low · .forgejo/workflows/presets.yml:13-19 · The check also runs on every PR that touches sword/src/install/, and it compares against CrossWire's live index. A CrossWire outage (exit 2) or an unrelated change in a module's size or licence turns an unrelated installer PR red. Consider keeping the PR run for presets/ and preset-check/** only, and leaving the install code to the weekly run and the local-server tests.
low · Cargo.lock and qt/Cargo.lock (winnow 0.7.15 and 1.0.4) · toml 0.9 brings in two versions of winnow (cargo deny warns on duplicates), and the release app now links serde, toml, ureq, rustls, ring and webpki-roots. Nothing in the PR states the effect on the click size budget (docs/07: 15 MB) or on start-up and memory, which the repo CLAUDE.md asks of a PR that adds them. ring also needs a C and asm toolchain for the arm64 build, and the Cargo.toml comment says so, but the PR gives no measured click size.
Delta review (medium), 9aa196a..3b616f1
Files reviewed:
3b616f1vsce21b29), core/src/db/history.rs, core/src/db/001_init.sql, core/src/db/002_tasks.sqlFindings: no high or medium findings.
Ok(()) if cancelledguard in install(). Removing the guard makes it fail (Done would be seen).3b616f1: only addsmod history;andpub use history::HISTORY_CAP;besidemod tasks. The migration list is unchanged (001 already holds the history table, 002 is tasks). tasks::Event and worker::Event are separate types, so there is no variant clash and no shared wiring.3b616f1; core lib 98 passed.Low:
d == "RawGenBook"is an exact match, while the LD checks are suffix matches. A conf with different ModDrv casing or spacing would take the Dir branch and mis-resolve the prefix. This is only a hypothetical for odd modules.Delta review (3b616f1..bbd9939, medium)
Checked: retry count (4 tries, 3 s apart, 3 sleeps), only Error::Network retries (Http/Protocol pass straight through; http_status_as_error(false) in sword/src/install/http.rs:50 means a 404 really arrives as Error::Http, so it stays a Problem), the RefCell bookkeeping (first failure kept, no overlapping borrow), and main.rs mapping run()'s Err to exit 2. cargo test -p utsb-preset-check passes.
low — tools/ci/preset-check/src/lib.rs:175 — after a module's HEAD has finally failed with Network, check() carries on and retries every remaining module — failure scenario: CrossWire's package host is down or blackholed (connect timeout 15 s): each module costs 4 x 15 s + 3 x 3 s = about 69 s, so 23 modules hold the single runner for about 26 min (about 3.5 min of that is pauses even with instant refusals) before reporting exit 2. Short-circuit: once
unreachableis set, return Err immediately for later modules without calling head.low — tools/ci/preset-check/src/tests.rs:177 — the tests do not count attempts, so "4 tries" is not pinned: a_head_that_gets_no_answer_once passes with 2 tries and the never-answers test passes with 1 or 100 — failure scenario: someone changes HEAD_TRIES or the guard
tries < HEAD_TRIESand nothing fails. Have heads() expose its call count and assert 4 for the persistent case and 1 for the 404 case (the 404 test's second answer Ok(500) only shows the 404 wins, not that it was not re-asked).low — tools/ci/preset-check/src/lib.rs:182 — a persistent network failure turns the whole result into Err, which drops Problems already found for other modules (e.g. a real licence change) — failure scenario: one real change plus one flaky HEAD gives exit 2 "could not check" with no mention of the change. Acceptable by design, but consider printing the collected problems alongside the error.
low — tools/ci/preset-check/tests/cli.rs — no CLI test for the new path (persistent Network on a package HEAD gives exit 2 and not 1); run()'s wiring of the closure into check_live is only covered by reading — failure scenario: a regression in the run() to check_live hookup would go unnoticed.
Delta review (bbd9939..c18f37e, medium)
No findings.
Checked: the early-return
unreachable.borrow()guard is dropped at the return, so the laterborrow_mut()cannot panic.get_or_insert_withkeeps the first module's failure as the message. Skipped modules return "not asked, as ..." andcheck()turns that into NoPackage problems, butcheck_livediscards them with the Err (the accepted note 3). Tests pass:cargo test -q -p utsb-preset-checkran 4 tests, all passing.asked.get() == HEAD_TRIESwith two modules shows the second module is never asked. The other tests count 2 and 1 HEAD calls.WIP: F1 starter sets 1/2: presets, resumable install tasks, preset CI checkto F1 starter sets 1/2: presets, resumable install tasks, preset CI checkMerge gate (step 8)
7950ada) are fixed in8bd9a64and present at head: migration race (process-wide lock, each step under BEGIN IMMEDIATE with user_version re-read in db.rs:187-201, test concurrent_opens_of_an_old_file_all_migrate, failed first) and the start_task hot loop (exec.rs halt() on a refused start_task or failed finish/fail/requeue, test hung on old code). The r2 re-review of8bd9a64found one low only, fixed in9aa196a.ce21b29andba394fd(cancel test, finish before Done, cumulative budget, three resume tests),7d99b6b(RawGenBook prefix) are covered by reviews 209 and 213.bbd9939andc18f37e(tools/ci/preset-check/src/lib.rs, tests.rs only) are covered by 214 and 215, and I re-read the lib.rs diff: retry only on Error::Network, status stays a Problem, later modules skipped once unreachable, no RefCell overlap.3b616f1is a merge of main (#27) that adds only history wiring.c5e1cd8is an empty trailer commit (Review-Confidence 55, Review-Tier trunk). Deltas 213-215 raised only lows: 1 declined with reason, 2 fixed, 2 deferred to #28 and #29 (416 path, preset-check exit 2 drops found problems; CLI test).c18f37e: PASS. Commit status success, all 5 contexts (CI / rust, CI / audit, CI / app, CI / click, Presets / preset-check).3b616f1; the later commits touch only tools/ci/preset-check. Targeted checks are named, with mutants shown failing: cargo-deny and cargo-audit on the new deps, and the preset-check job (green live in CI, exit 1 on a broken copy). Elevated: targets written down (CrossWire GET/HEAD only; throwaway DBs for migration 002; local mock API for the issue step), and the effect was read back (live "ok: 5 sets, 23 modules", migration test, mock-issue outcomes). The body lists the gaps honestly under Not covered (no arm64 run, click size unmeasured, 416 path) and none is a tier gate. Both are nothing-wired-yet items for part 2.Review-Confidence is 55; that is the author's score and is not what this gate checks. Approving head
c5e1cd8.