Download Resources from the Publish API - #69
Conversation
* Ktor client with OkHttp (android) / CIO (jvm) engines and shared HttpTimeout bounds sized for the largest published file (~1MB) * Fetch index.json, HEAD-probe published files, stream downloads; per-file error text instead of thrown exceptions * State holder exposing remote info with error text, keeping known metadata on transient refresh failures * Unit tests over MockEngine for probing, failure isolation and the state holder
* Persisted in AppPreferences, defaults to the official publish URL * Edit dialog with reset-to-default in general settings
* Extract the serial task queue from DatabaseManageViewModel into ImportTaskQueue; downloads log start and failure under per-resource tags instead of falling through to the queue's raw fallback * Download section with remote status panel: per-file version and built_at display, manual refresh, items disabled while fetching
* Validate the downloaded database the same way as manual import, then swap into place via a staging file and atomic move so an interrupted copy cannot truncate the existing ih.db * Manual import now uses the same staged swap
* Refresh remote info automatically when the base URL changes; a refresh in flight queues a rerun with the new URL instead of the change being dropped by the isFetching guard * Disable download items while remote status is unknown; validate the base URL (http/https) in settings, default the UI state * ImportTaskQueue: isWorking stays on between queued tasks, parent Job required explicitly instead of `!!` * Download ih.db straight to the staging file, dropping the intermediate cache copy; note the application-context contract on the OCR VM * Deterministic holder/client tests: MockEngine pinned to the test scheduler, plain scope instead of backgroundScope (advanceUntilIdle stops on background-only tasks), chunked download, HEAD/GET and index edge-case coverage
The four HEAD probes run concurrently, so their order is not part of the contract; asserting the exact sequence failed on CI. Collect URLs from the engine's thread-safe requestHistory instead of racing on a shared list.
|
/opencode review this pr |
|
I reviewed the full diff plus supporting code, compiled VerdictSolid work: clean layering (shared client / app consumers), per-resource failure isolation with error text, good MockEngine unit coverage, and the staged-swap / atomic-move idea is the right instinct. A few correctness/robustness concerns before merge. Issues1.
|
* atomicMove replaces an existing destination (kotlinx-io passes REPLACE_EXISTING), so drop the pre-delete that could leave ih.db missing between the delete and the move * Serialize the two ih.db writers through the UI: manual import is disabled while a download runs and vice versa * Clear probed info when the base URL changes and discard results from a probe that started against the previous URL * Cap every resource at 20MB: fail fast on a declared oversized body and bound the streamed byte count for chunked responses
java.nio.file needs API 26+ and cannot be desugared, so kotlinx-io's atomicMove always throws UnsupportedOperationException on API 24/25 and the whole ih.db swap (manual import and network download) fails there. Catch it and retry via File.renameTo, which maps to rename(2) on every Android level: atomic and replacing an existing target, given both paths sit on the same filesystem.
Download errors were only recorded in the import-log sheet, which users have no reason to open after a failed download; the item silently returned to idle. Track the last failure per resource, show it via the same inline error slot the OCR flow uses, and clear it on re-submission.
Android 9+ blocks cleartext traffic by default, so an http:// base URL passes validation but every probe and download fails with a generic network error that gives the user no clue about the cause. Show a hint in the edit dialog when the URL is http.
The fetch guard (check-then-act on isFetching) was not atomic, so two concurrent refresh() calls could both pass it and probe in parallel, flickering isFetching and double-fetching. Worse, the collector's reset (gen bump + state clear + rerunPending) was not ordered against a refresh coroutine's publish: interleaved just right, the reset could re-write isFetching=true after the publish cleared it while rerunPending was set too late to be consumed, leaving the holder stuck on isFetching=true with no fetch running. Serialize the guard, the publish, the collector reset and the rerun bookkeeping under one Mutex (network fetch stays outside it), and drop the now-unneeded @volatile markers. The generation is captured in the same critical section that raises the guard. Extend the in-flight-refresh test to two overlapping calls.
…size - OCR ih.db download/import and DatabaseManage download submission set their busy flags before dispatching to the IO dispatcher. The flag was previously raised inside the coroutine, so a double tap in the dispatch window passed the guard twice and could race two writers on the shared ih.db staging file. - ImportTaskQueue rethrows CancellationException instead of logging it as a task failure: the log append itself throws on the already-cancelled coroutine, and ViewModel destruction is not a task error. - The HEAD probe now rejects a file whose declared Content-Length exceeds the resource limit, surfacing the reason on the item instead of failing only when the download hits the streamed size bound. Covered by a new client test (at-limit Content-Length stays available).
|
/opencode follow the new commits (starting from 657ba34). |
VerdictFollow-up commits (657ba34 → 61dcfb3, 7 commits) are solid. I read the full range plus supporting code, verified kotlinx-io 0.9.0 move semantics from the local bytecode, ran Issue-by-issue#1 (atomicMove overwrite / data-loss window) — Fixed. Confirmed in the jar: kotlinx-io's #2 (staging-path writers) — Fixed for the two UI writers, but only two of three. Manual import and remote download are now mutually exclusive via busy flags raised synchronously before dispatch ( #3 (stale info after base-URL change) — Fixed. The collector clears info/error and bumps a generation on URL change; in-flight probes against the old URL can't publish, and the rerun is queued ( #4 (navigation cancels import) — Confirmed intentionally ignored. #5 (size bound) — Fixed. 20MB enforced at probe time (HEAD First-review nits: inline DB-manage download errors (9a31a4e), cleartext-http hint (3c22078), and the Other observations
|
The download item is disabled via recomposition, which lags the click: a fast double tap passed enqueueDownloadTask twice and enqueued the resource twice (the busy flag itself is already set synchronously). The serial queue keeps it from corrupting anything, but the duplicate download and import are pure waste. Guard on set membership, matching the OCR flow's in-VM guards.
1 participant
Summary
Adds the ability to download packlist, songlist and the chart info database from the publish API (Database Manage), and the image hashes database from the network (OCR Dependencies).
Changes
shared/core/api: Ktor-basedArcaeaResourcesApiClient(index.json fetch, HEAD probes, streamed downloads) andRemoteResourcesInfoStateHolder; per-platform engines (OkHttp / CIO) with shared timeout bounds sized for the largest published file (~1MB)Notes