feat: Optimize download performance for external storage - #1785
Open
joshuatam wants to merge 11 commits into
Open
feat: Optimize download performance for external storage#1785joshuatam wants to merge 11 commits into
joshuatam wants to merge 11 commits into
Conversation
Optimized download changes: - Debouncing download progress persistence to reduce disk I/O. - Switching to `FileChannel` for file writes and `Path.deleteRecursively()` for robust file system operations across various download managers (Epic, GOG). - Dynamically adjusting download and decompression concurrency based on CPU cores for better resource utilization. - Streamlining download status message updates in `DownloadInfo` and UI components. - Adding an option to skip large file allocation for Steam downloads to external storage. Also updates the JavaSteam dependency to 1.8.0.1-25-SNAPSHOT.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds debounced download-state persistence, synchronous status access, listener-driven progress rendering, positioned channel writes for Epic and GOG assembly, bounded download limits, redirected-staging allocation handling, updated JavaSteam snapshots, and persistence tests. ChangesDownload I/O and progress
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DownloadInfo
participant DownloadsViewModel
participant LibraryAppScreen
participant PersistenceFile
DownloadInfo->>PersistenceFile: schedule debounced persistence
DownloadInfo->>DownloadsViewModel: provide current status
DownloadsViewModel->>LibraryAppScreen: expose active download
LibraryAppScreen->>DownloadInfo: register progress listener
DownloadInfo->>LibraryAppScreen: report progress and status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/data/DownloadInfo.kt`:
- Around line 292-314: Serialize all persistence scheduling in
persistBytesDownloaded using a single synchronization mechanism, reserve the
next write time before launching I/O, and ensure rapid updates cannot start
overlapping writes. Update clearPersistedBytesDownloaded to cancel or invalidate
any pending delayed persistence so completion cannot recreate the deleted file,
and add coverage for rapid updates plus completion during the debounce interval.
In `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Line 267: Make recursive cache cleanup best-effort by wrapping
deleteRecursively calls at EpicDownloadManager.kt:267,
EpicDownloadManager.kt:411, EpicDownloadManager.kt:503, and
GOGDownloadManager.kt:484 in try/catch blocks; ignore cleanup failures so
subsequent database, marker, DLC, overlay, manifest, and installed-game
finalization steps continue.
- Around line 1239-1242: Drain every FileChannel.write(ByteBuffer) call before
updating progress counters: in EpicDownloadManager.kt lines 1239-1242 and
GOGDownloadManager.kt lines 1607-1615, 1626-1639, and 1657-1660, wrap each write
in a loop that continues until the buffer has no remaining bytes, including
zero-byte writes as required by the channel contract. Preserve the existing
remaining/totalBytesWritten updates, but perform them only after the full buffer
is written.
In `@app/src/main/java/app/gamenative/service/epic/EpicService.kt`:
- Around line 243-249: Handle deleteRecursively failures across
app/src/main/java/app/gamenative/service/epic/EpicService.kt lines 243-249,
app/src/main/java/app/gamenative/service/SteamService.kt lines 1348-1350, and
app/src/main/java/app/gamenative/service/gog/GOGManager.kt lines 501-511: in
EpicService.deleteGame, catch partial-delete exceptions and still complete
uninstall, marker, container, and event updates before returning failure; in
SteamService.deleteApp, catch the deletion failure around database cleanup and
return false; in GOGManager.deleteGame, record failed paths, continue
marker/database/container/event cleanup, and return failure instead of
propagating the exception.
In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt`:
- Around line 597-615: Update the listener setup around the progressListener and
its effects to use a single DisposableEffect(downloadInfo) that registers the
current listener and removes that same listener on cleanup. Ensure the callback
uses the progress argument, or rememberUpdatedState for isDownloading and other
changing inputs, so the Unpacking state reflects current values and old
listeners are not retained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 51708829-22de-4290-82a1-56b2a061ad2c
📒 Files selected for processing (11)
app/build.gradle.ktsapp/src/main/java/app/gamenative/data/DownloadInfo.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/epic/EpicService.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/ui/model/DownloadsViewModel.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/java/app/gamenative/utils/DownloadSpeedConfig.ktgradle/libs.versions.toml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
14 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt:1615">
P1: GOG downloads can produce corrupted files when a `FileChannel.write` performs a short write, because these calls advance the accounting as if the entire buffer was persisted. Drain each buffer with `while (byteBuffer.hasRemaining())` at all three new write sites before updating the byte count.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt:17">
P2: Low-core devices can now run several download and decompression workers despite the CPU-based tuning, increasing contention and memory pressure and potentially making downloads less stable. Making the minimum depend on `cpuCores` (for example, retaining a minimum of 1) would preserve the intended scaling on constrained devices.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGManager.kt:505">
P1: A deletion failure can leave the GOG game marked as installed even after uninstall has partially removed its files, because `Path.deleteRecursively()` exits the loop by throwing before marker and database cleanup. Catch the per-path deletion exception, add the path to `failedPaths`, and continue cleanup so installation state is reconciled.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt:267">
P2: Path.deleteRecursively() can partially delete the tree and still throw an IOException if any entry fails to delete. Since this call is not wrapped in try/catch, a failure here will now propagate and skip the subsequent database/marker finalization steps (base-game DB updates, DLC persistence, overlay completion) that should still run even if cache cleanup fails. Consider wrapping this call in a try/catch so cleanup failures are best-effort and don't block completion logic.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt:1242">
P1: Epic file assembly can silently produce truncated or corrupted files when `FileChannel.write` performs a partial write. Loop until `byteBuffer` has no remaining bytes before advancing the input-progress counter.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt:287">
P2: Status messages can remain stale in the downloads list because this new read accesses a non-volatile field concurrently updated by download workers. Keeping the `StateFlow` read or making `currentStatusMessage` thread-safe (for example, `@Volatile`) preserves visibility.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:597">
P2: Download status text can become stale, notably leaving the UI blank instead of showing unpacking when the download reaches 100%, because the registered callback uses captured progress rather than its current listener argument. Use the emitted progress value and/or `rememberUpdatedState` for changing inputs while keeping one lifecycle-managed listener.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:606">
P3: The new ETA and unpacking labels are always rendered in English, so localized users see untranslated text. Add formatted strings to resources and obtain them with `stringResource` instead of embedding UI text in the composable.</violation>
</file>
<file name="app/src/main/java/app/gamenative/data/DownloadInfo.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:29">
P2: The new debounce state `lastPersistTime` (and `persistenceJob`) is read in `persistBytesDownloaded` on the caller's thread but written in `writePersistedBytes` which runs on `ioScope` (Dispatchers.IO), with no `@Volatile`/synchronization. This is a cross-thread data race: the caller can read a stale `lastPersistTime`, wrongly take the immediate-write branch (writing far more often than the intended 10s debounce and undermining the I/O reduction this PR targets), or schedule redundant overlapping writes. Since the debounce's whole purpose is to cut disk I/O, guarding these fields is worth doing. Marking the fields `@Volatile` (or serializing these mutations under a lock / on a single scope) would make the debounce behave as intended.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:312">
P1: Completed or cancelled downloads can leave or recreate a stale `.DownloadInfo/bytes_downloaded.txt`, causing a later resume to load obsolete progress. Persistence needs serialized/awaitable completion before cancellation and checkpoint cleanup, or cleanup must cancel and drain pending writes.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:328">
P2: Rapid progress snapshots can still issue multiple disk writes, so the claimed 10-second debounce is not guaranteed. Reserve/update the persistence timestamp and coalesce requests under synchronization before launching the IO write.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:1348">
P1: Uninstall can leave stale database records when any file cannot be deleted: `Path.deleteRecursively()` throws, so the cleanup transaction is skipped. Handling the deletion failure and returning `false` (or otherwise continuing with an explicit partial-delete state) would keep filesystem and database state consistent.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/SteamService.kt:1879">
P2: Internal-storage updates can now skip large-file preallocation whenever the external-storage preference is enabled, even though `getAppDirPath()` resolved the game to an internal install. Derive this option from the resolved `appDirPath` (for example, whether it is outside `baseDataDirPath`) rather than from the global preference.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/epic/EpicService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/epic/EpicService.kt:246">
P2: Path.deleteRecursively() can partially delete files and still throw. Since this replaces the old boolean-returning deleteRecursively() check without any exception handling, a partial-delete failure will now propagate and skip the marker removal and remaining uninstall cleanup steps that follow. Wrap this in try/catch, log/record any failed paths, and continue with marker/DB/container cleanup regardless.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/data/DownloadInfoTest.kt (1)
62-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake persistence tests deterministic.
These tests drive debounce and pending write completion through wall-clock delays (
delay(11_000)plus severaldelay(100)calls), which can still race withDispatchers.IOwrites. Expose a testable persistence dispatcher/clock or completion hook so the tests can advance and await persistence without adding 33+ seconds of wall-clock time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/app/gamenative/data/DownloadInfoTest.kt` around lines 62 - 166, Make the persistence tests deterministic by exposing an injectable persistence dispatcher, clock, or completion hook in DownloadInfo, then update the debounce-related tests around persistBytesDownloaded and clearPersistedBytesDownloaded to advance or await scheduled writes explicitly instead of using delay(100) and delay(11_000). Preserve assertions for immediate writes, debounced updates, and cancellation without relying on wall-clock timing or Dispatchers.IO races.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 1236-1241: Update the byteBuffer write loop in EpicDownloadManager
to detect FileChannel.write returning 0, preventing indefinite retries with no
progress. Use a bounded retry strategy or throw an IOException after
zero-progress writes, while preserving normal handling when bytes are written
and allowing the download to fail rather than hang.
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Line 1345: Update the cleanup flow around File(appDirPath).deleteRecursively()
to check its boolean result; when deletion fails, return false immediately
before removing the marker or deleting database rows, while preserving the
existing successful cleanup path.
- Line 1876: Update the path comparison used to assign skipLargeFileAllocation
in the DepotDownloader configuration to compare absolute, normalized Paths for
both appDirPath and DownloadService.baseDataDirPath. Preserve the existing
startsWith boundary check and invert its result so external locations continue
to enable large-file allocation.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/data/DownloadInfoTest.kt`:
- Around line 62-166: Make the persistence tests deterministic by exposing an
injectable persistence dispatcher, clock, or completion hook in DownloadInfo,
then update the debounce-related tests around persistBytesDownloaded and
clearPersistedBytesDownloaded to advance or await scheduled writes explicitly
instead of using delay(100) and delay(11_000). Preserve assertions for immediate
writes, debounced updates, and cancellation without relying on wall-clock timing
or Dispatchers.IO races.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 702811a2-05bb-4b9d-852f-c4607cceccd4
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/data/DownloadInfo.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/test/java/app/gamenative/data/DownloadInfoTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/data/DownloadInfo.kt
- app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
Contributor
There was a problem hiding this comment.
12 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt:1237">
P3: File assembly performs an unnecessary self-copy of every 64 KiB block before writing, adding memory traffic in the hot download path. Limiting the wrapped buffer to `bytesRead` avoids the copy while preserving partial-write handling.</violation>
</file>
<file name="app/src/test/java/app/gamenative/data/DownloadInfoTest.kt">
<violation number="1" location="app/src/test/java/app/gamenative/data/DownloadInfoTest.kt:74">
P2: These tests race the asynchronous disk write: persistBytesDownloaded returns immediately after launching on Dispatchers.IO, but the tests only sleep 100ms before reading the file. On slow/loaded CI the async write may not have landed, making the first-value assertions flaky. Await the write deterministically (e.g. expose a suspend/joinable variant) instead of assuming a fixed sleep is enough.</violation>
<violation number="2" location="app/src/test/java/app/gamenative/data/DownloadInfoTest.kt:92">
P3: These three persistence tests each sleep ~11 seconds of real time because they wait out the hardcoded 10,000 ms debounce, adding roughly 33 seconds to the unit test run. Consider making the debounce window injectable (constructor default) so the tests can use a short delay, and/or replace the repeated `delay(11_000)` with a named constant so the relationship to the debounce window is explicit and documented.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:625">
P1: The download-time label logic produces wrong output and leaks listeners. The `progressListener` closure is created fresh on every recomposition but only registered when `downloadInfo` changes, so the registered closure captures stale `isDownloading`/`downloadProgress` values; when progress reaches 100% the "Unpacking..." branch and the `downloadProgress in 0f..1f` branch evaluate against the initial (≈0) progress instead of the live value, yielding incorrect labels. Additionally, add and remove are split across two different Effects: removal only happens in `DisposableEffect(Unit)` when leaving composition, so if `downloadInfo` changes identity the old DownloadInfo's listener is never removed (leaking and continuing to write to `downloadTimeLeftText`) while a new one gets added. Register and remove within a single `DisposableEffect(downloadInfo)` and read the up-to-date `isDownloading`/`downloadProgress` via `rememberUpdatedState`, matching the existing pattern in LibraryListCard.kt.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:626">
P2: Already-running downloads do not show their current ETA or status when this screen is opened because listener registration never performs an initial update. Invoke the listener once with `downloadInfo.getProgress()` after registering, or derive the text directly from current state.</violation>
</file>
<file name="app/src/main/java/app/gamenative/data/DownloadInfo.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:49">
P2: Download status text can remain stale when service and UI callbacks run on different threads because the replacement for `MutableStateFlow` is an unsynchronized field. Retain a `StateFlow`/`AtomicReference`, or mark the field volatile if the listener-based API is intentional.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:314">
P2: Concurrent progress snapshots can bypass the debounce and issue multiple immediate disk writes because the reservation check and update are not atomic. Reserve the slot with a CAS or mutex and coalesce callers under the same synchronization.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:316">
P2: A download can recreate the deleted progress file after completion. The immediate-write branch of `persistBytesDownloaded` launches a coroutine that is never stored in `persistenceJob` and never checks `persistenceGeneration`, so `clearPersistedBytesDownloaded` cannot cancel it and does not invalidate it via the generation counter — an in-flight write that starts just before the clear can call `writePersistedBytes` after the file was deleted and recreate `bytes_downloaded.txt`. The new tests only validate the generation guard on the delayed/waiting path, not this one. Apply the same generation check used by the delayed-branch coroutine so both paths respect the clear.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt:1636">
P3: The `ByteBuffer.wrap(outputBuffer)` + `clear()` + `put(outputBuffer, 0, count)` + `flip()` sequence performs a redundant self-copy: the buffer's backing array *is* `outputBuffer`, and after `clear()` the position is 0, so `put(outputBuffer, 0, count)` copies the array's own bytes back onto itself — a no-op that does pointless work on every decompressed chunk, which is notable for a change whose stated goal is reducing I/O and memory overhead. The same redundant `clear/put/flip` dance is repeated in all three write paths in this function. The write is still correct (the channel ends up receiving `count` bytes), but the `put` should be dropped in favor of just setting the buffer limit, which makes the intent clear and saves the copy. Using `byteBuffer.clear(); byteBuffer.limit(count)` (position is already 0 after `clear`) is equivalent and simpler.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:1347">
P2: This added `true` makes `deleteApp` report success even when `File.deleteRecursively()` fails to remove the game files (e.g. a file is locked or the filesystem refuses deletion). Because callers in SteamAppScreen and ContainerStorageManager use this return value to decide whether to show 'uninstall success' vs 'uninstall failed' (and whether to emit the uninstall event/snackbar), a real disk-deletion failure is now silently masked — the user is told the uninstall succeeded and the DB record is removed even though the files remain on disk, preventing a clean retry. Rather than unconditionally returning `true`, consider returning the actual deletion result (or throwing/handling a failure distinctly) so the deletion-reliability goal of this PR is preserved.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/SteamService.kt:1876">
P3: This duplicates the 'external vs internal install' decision already made on the `chunkStagingRedirectDir` line above it, using a different comparison method (Path.startsWith vs String.startsWith). Keeping two parallel implementations of the same predicate in the same function risks them drifting apart. Consider extracting a single helper (e.g. `private fun isInternalInstall(path: String) = Paths.get(path).startsWith(Paths.get(DownloadService.baseDataDirPath))`) and reusing it for both the chunk-staging redirect and the skipLargeFileAllocation decision.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/service/SteamService.kt:1876">
P2: Paths.get(appDirPath).startsWith(...) compares raw path segments without resolving relative components like '..' or symlinks. If appDirPath (which can come directly from a custom install path) contains such segments, this check can misclassify an external-storage path as internal, incorrectly disabling skipLargeFileAllocation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/data/DownloadInfo.kt`:
- Around line 285-300: Update DownloadInfo.persistBytesDownloaded to use a
bounded deadline rather than resetting the ten-second delay for every progress
request; in DownloadInfo.kt:285-300 schedule at most one checkpoint window. In
DownloadInfo.kt:341-341, invalidate and serialize pending writes before deleting
the checkpoint, and in DownloadInfo.kt:351-358 flush or await the cancellation
snapshot before shutdown. In AmazonService.kt:506, EpicService.kt:296 and 510,
and GOGService.kt:258 and 439, preserve cancellation progress before shutdown,
explicit cleanup, or finally-triggered cleanup by invoking the appropriate
DownloadInfo cancellation persistence flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1187b6a1-18cb-490b-8406-853ad070ca4e
📒 Files selected for processing (10)
app/build.gradle.ktsapp/src/main/java/app/gamenative/data/DownloadInfo.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/amazon/AmazonService.ktapp/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/epic/EpicService.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/test/java/app/gamenative/data/DownloadInfoTest.ktgradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- app/build.gradle.kts
- gradle/libs.versions.toml
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
- app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt
- app/src/test/java/app/gamenative/data/DownloadInfoTest.kt
Contributor
There was a problem hiding this comment.
10 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt:14">
P3: The new concurrency policy has no focused tests for tier selection, CPU scaling, or low-core clamps, so regressions in download/decompression limits can reach all three managers unnoticed. A `DownloadSpeedConfig` test covering each setting and representative core counts would make this optimization safer to change.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt:17">
P2: The same `downloadSpeed` preference now yields different concurrency depending on which download path is used. The thread-count logic here was introduced/updated with new ratios, but `WorkshopManager.computeDownloadThreads()` still duplicates this calculation using the old ratios (0.6/0.2 ... 2.4/0.8 with `coerceAtLeast(1)`), so the two implementations have already drifted and will keep diverging. Consider extracting a single source of truth (have WorkshopManager delegate to `DownloadSpeedConfig`) so all managers honor one consistent configuration, and add a unit test covering the new limits table per speed tier.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt:25">
P2: Low-core devices now run at least six download workers and two decompression workers for the default setting, even when the CPU-scaled calculation yields one or two workers. Bounding the minimums by available cores (or retaining the previous one-worker floor) would avoid saturating low-end devices.</violation>
</file>
<file name="app/src/test/java/app/gamenative/data/DownloadInfoTest.kt">
<violation number="1" location="app/src/test/java/app/gamenative/data/DownloadInfoTest.kt:95">
P2: These tests burn ~55s of real wall-clock time sleeping 11s to satisfy the hardcoded 10s debounce, since PERSIST_DELAY_MS is a private constant that can't be shortened or made virtual. That drags out every CI run and hard-couples the tests to the 10s value — if the delay is ever changed the sleeps silently stop matching and the tests become flaky or wrong. Consider making the debounce delay injectable (constructor param or internal setter) and driving the tests with a tiny/virtual delay instead of real sleeps, keeping the timing wall-clock-independent.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:606">
P3: ETA and unpacking messages bypass Android string resources, so these new download labels cannot be localized. Add resource entries with format arguments and render them through `stringResource`.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:625">
P2: Download status callbacks can leak after leaving the game screen and can display stale phase text because the listener is added per `downloadInfo` change but removed only for the initial value, while its lambda captures the initial `downloadProgress` and `isDownloading`. Register and remove the same listener in `DisposableEffect(downloadInfo)`, using the callback progress or otherwise reading the latest state.</violation>
</file>
<file name="app/src/main/java/app/gamenative/data/DownloadInfo.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:294">
P2: Completed downloads can have `bytes_downloaded.txt` recreated after cleanup when an already-running delayed write races with deletion. Coordinate writes and clearing, or invalidate the task generation and check it before writing.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:357">
P1: Cancelled downloads can lose their latest resume checkpoint because `shutdown()` cancels the debounced write immediately after `cancel()` schedules it. Flush the pending snapshot before shutting down, or allow the queued task to drain during shutdown.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:1348">
P2: Failed filesystem deletion is now reported as success, so callers can remove the Steam app record while its install directory remains on disk. Preserve and return the deletion result (or abort database cleanup when deletion fails).</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt:1609">
P3: The new FileChannel-based write logic is repeated in four places across the two managers (three times in GOGDownloadManager.decompressChunkToFile and once in EpicDownloadManager). The ByteBuffer wrap + clear/limit + drain loop is a small but genuinely duplicated file-operation helper. Extracting a single shared utility (e.g. a FileUtils extension `fun FileChannel.writeAll(bytes: ByteArray, length: Int)`) and calling it from each path would remove the duplication, keep the byte-writing semantics in one reviewed spot, and make future tweaks (forcing, buffering, error handling) consistent across managers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…r writePersistedBytes" This reverts commit 690827d.
Contributor
There was a problem hiding this comment.
9 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt:17">
P2: This change reworks the core concurrency tuning that every download manager (Steam, Epic, GOG) depends on, introducing new CPU-core math with per-setting floors and caps (including the low-core floor behavior described above). There's no unit test covering these new paths, and the PR's test additions only cover debounced persistence and download state cleanup. Adding a focused DownloadSpeedConfig test (verifying min/max capping across representative core counts and each speed setting) would protect against regressions in this cross-manager behavior that's otherwise easy to break unintentionally.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/DownloadSpeedConfig.kt:22">
P2: Medium, fast, and blazing downloads can oversubscribe low-core devices: a one-core device gets 2 or 3 decompression workers because of these lower bounds. Keeping the lower bound at 1 for CPU-constrained devices would make the advertised CPU-based tuning effective and avoid competing assembly workers.</violation>
</file>
<file name="app/src/test/java/app/gamenative/data/DownloadInfoTest.kt">
<violation number="1" location="app/src/test/java/app/gamenative/data/DownloadInfoTest.kt:74">
P2: These tests depend on wall-clock timing that can flake and is slow. The 100ms delay after a background IO write does not guarantee writePersistedBytes finished under CI load, so assertEquals(100L) may intermittently read 0; and 3 tests each sleeping 11s makes the suite ~33s of real time. Recommend injecting the debounce interval (or a virtual clock via kotlinx-coroutines-test) and awaiting the write coroutine deterministically instead of fixed sleeps.</violation>
</file>
<file name="app/src/main/java/app/gamenative/data/DownloadInfo.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:315">
P2: Parallel downloads can bypass the debounce and issue multiple persistence writes in the same interval because `nextWriteTime.get()` and `.set()` are not an atomic reservation. Serialize the reservation and `persistenceJob` updates, or use a compare-and-set/Mutex-based scheduler.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:317">
P2: The immediate-write path launches the `writePersistedBytes` coroutine without storing the job in `persistenceJob` and without checking `persistenceGeneration`. If this write is in-flight on the IO dispatcher when `clearPersistedBytesDownloaded()` runs (it increments the generation and deletes the file), the in-flight write can still complete afterwards and silently re-create the persistence file — exactly what the generation-invalidation logic was meant to prevent. There is a similar window where an already-running delayed write isn't interruptible by `persistenceJob?.cancel()`. Consider guarding the actual write with the generation value captured at schedule time (and storing/joining the immediate job) so a completed download's file isn't recreated.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:625">
P2: Opening a screen for an already-running download can show no ETA or status until the next progress update. Initialize the displayed text from the current `DownloadInfo` state when attaching the listener.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:625">
P2: The new `progressListener` closure is registered only once per `downloadInfo` via `LaunchedEffect(downloadInfo)`, but the closure captures `downloadProgress` and `isDownloading` from the composition at the moment of that one run. As download progress changes, recomposition creates a new `progressListener`, yet it is never re-registered (the effect key is only `downloadInfo`), so the listener actually invoked by `emitProgressChange()` always reads the *stale* first-captured `downloadProgress`/`isDownloading`. Since `downloadTimeLeftText` is now assigned only inside this listener (the old `remember(...)` recomputation was removed), the displayed progress text can become wrong: e.g. the `isDownloading && downloadProgress >= 1f` "Unpacking..." branch never triggers because the registered closure still sees the initial (<1) progress, and the status-message fallback is evaluated against a stale progress value. The original `remember(displayInfo.appId, downloadProgress, downloadInfo, isDownloading, downloadStatusMessage)` recomputed with fresh values; the new listener-driven approach regresses this. Also, when `downloadInfo` changes, `LaunchedEffect` re-runs `addProgressListener` and adds a new listener without removing the prior one (the `DisposableEffect(Unit)` only removes the first-composition closure), so listeners accumulate on old `DownloadInfo` instances.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt:1237">
P3: The streaming `FileChannel.write` loop (wrap buffer, clear/limit, write-until-drained) is now copy-pasted across EpicDownloadManager and three places in GOGDownloadManager. Consider extracting a small shared helper (e.g. a `writeFully(FileChannel, ByteArray, len)` utility) so the write semantics stay consistent and any future fix (e.g. handling short writes differently) is applied in one place.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:1347">
P2: deleteApp now always returns true for non-imported games, so a failed recursive deletion on disk is reported as a successful uninstall. Callers rely on this Boolean to decide whether to show a success snackbar/refresh the library (SteamAppScreen.kt) or to fail the uninstall (ContainerStorageManager.kt), so a real deletion failure (locked file, permission error, partially-deleted folder) would now be silent and leftover files would remain while the library shows the game as removed. The trailing `true` overwrites `deleteRecursively()`'s error signal; consider returning its result (or handling the failure explicitly) instead of unconditionally returning `true`. Note this also appears unrelated to the PR's download-performance scope.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…-download-performance-for-external
8 tasks
Owner
There was a problem hiding this comment.
Hi @joshuatam - are the changes in this file improving the speed as well? If not I think we should undo it.
If it is for fixing GOG, we can fix the issue in GOGDownloadManager.kt:937 - set the status without the separate setProgress call so a chunk costs one emit. Thoughts?
Contributor
Author
There was a problem hiding this comment.
Hi @joshuatam - are the changes in this file improving the speed as well? If not I think we should undo it.
If it is for fixing GOG, we can fix the issue in GOGDownloadManager.kt:937 - set the status without the separate setProgress call so a chunk costs one emit. Thoughts?
It is needed as DownloadInfo changed the way to emit progress.
If you check DownloadsViewModel.kt carefully, the same getStatusMessageFlow() is consuming twice, together with the same call in LibraryAppScreen, there are totally 3 locations listen to the same progress. Changing to listener pattern can make the UI smooth and reduce the chance on updating the same UI elements simultaneously in DownloadsViewModel.kt (could be somehow causing ANR)
If you concern about the mention in this https://github.com/utkarshdalal/GameNative/pull/1785/changes/BASE..7d00f1758010abaeb4a294a411d20594498bcf12#r3695608006.
I would say it is already handled by progressListener https://github.com/utkarshdalal/GameNative/pull/1785/changes/BASE..7d00f1758010abaeb4a294a411d20594498bcf12#diff-27870a3ee8b1cca032461ee90916f564946eb57c690cd5b7cb57e95b64031427R626
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Optimized download changes:
FileChannelfor file writes andPath.deleteRecursively()for robust file system operations across various download managers (Epic, GOG).DownloadInfoand UI components.Also updates the JavaSteam dependency to 1.8.0.1-26-SNAPSHOT.
Recording
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Optimizes external storage downloads across Steam, Epic, and GOG for faster, smoother installs. Reduces disk I/O, improves write reliability, auto-tunes concurrency, and bumps
javasteamto1.8.0.1-26-SNAPSHOT.Performance
FileChannelfor Epic/GOG writes with correctByteBufferhandling.DownloadSpeedConfigwith explicit min/max per speed tier.New Features
Written for commit 7d00f17. Summary will update on new commits.
Summary by CodeRabbit