fix: Optimize GOG and EPIC download#1649
Conversation
|
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:
📝 WalkthroughWalkthroughEpic and GOG download flows now pre-allocate files in chunk-sharing order, log cache and expected disk usage while waiting on pending chunks, and GOG streams chunk decompression directly into the destination file. ChangesChunk scheduling, disk logging, and streaming assembly
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt (1)
1030-1065: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEmit chunks in
sortedFilesorder if the chunk-usage heuristic should matter.
sortedFilescurrently only affects pre-allocation;chunkQueueis still built from the originalfilesorder, so download/assembly stays unsorted. Emitting deduplicated GUIDs fromsortedFileswould make the “least shared first” ordering take effect.🤖 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/main/java/app/gamenative/service/epic/EpicDownloadManager.kt` around lines 1030 - 1065, The chunk-usage sorting in EpicDownloadManager’s file processing only changes pre-allocation today, while the actual chunk emission still comes from chunkQueue in the original files order. Update the chunk collection/emission flow so deduplicated chunk GUIDs are emitted from sortedFiles instead of the unsorted source, using the existing sortedFiles loop and networkChunkFlow.emit path, so the “least shared first” heuristic affects download/assembly order.
🤖 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 1086-1098: The storage stats block in EpicDownloadManager’s
pending-chunks loop is doing an expensive full installDir scan every 5s just to
build a debug message. Move the
chunkCacheSize/calculateDirectorySize(installDir) work behind a debug-only guard
around the Timber.tag("EPIC").d call, or otherwise suppress it when debug
logging is off. If you keep the log, prefer incremental size tracking or much
less frequent sampling so the recurring installDir traversal does not contend
with active downloads.
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 1040-1052: The install-size logging in GOGDownloadManager is doing
an expensive recursive scan of installDir every 5 seconds, even when debug logs
are not enabled. Move the size calculation behind a Timber debug check (or
equivalent) and avoid calling calculateDirectorySize(installDir) unless the log
will actually be emitted; if possible, cache or compute these sizes
incrementally instead of rescanning. Apply the same pattern in
EpicDownloadManager where the same disk-usage debug logging exists.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 1030-1065: The chunk-usage sorting in EpicDownloadManager’s file
processing only changes pre-allocation today, while the actual chunk emission
still comes from chunkQueue in the original files order. Update the chunk
collection/emission flow so deduplicated chunk GUIDs are emitted from
sortedFiles instead of the unsorted source, using the existing sortedFiles loop
and networkChunkFlow.emit path, so the “least shared first” heuristic affects
download/assembly order.
🪄 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
Run ID: 807d7627-557e-43af-a190-d711c9d9d8fd
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 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:1580">
P2: The new streaming `decompressChunkToFile` path and its edge cases (non-zero offsets, zlib EOF, size/MD5 mismatch, uncompressed chunks) are not covered by any new or existing test. Per project guidance, complex core-service changes should include focused tests to catch regressions and future refactors.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * @return Result indicating success or failure | ||
| */ | ||
| private fun decompressChunk(compressedBytes: ByteArray, chunk: FileChunk): Result<ByteArray> { | ||
| private fun decompressChunkToFile( |
There was a problem hiding this comment.
P2: The new streaming decompressChunkToFile path and its edge cases (non-zero offsets, zlib EOF, size/MD5 mismatch, uncompressed chunks) are not covered by any new or existing test. Per project guidance, complex core-service changes should include focused tests to catch regressions and future refactors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt, line 1580:
<comment>The new streaming `decompressChunkToFile` path and its edge cases (non-zero offsets, zlib EOF, size/MD5 mismatch, uncompressed chunks) are not covered by any new or existing test. Per project guidance, complex core-service changes should include focused tests to catch regressions and future refactors.</comment>
<file context>
@@ -1564,62 +1568,107 @@ class GOGDownloadManager @Inject constructor(
+ * @return Result indicating success or failure
*/
- private fun decompressChunk(compressedBytes: ByteArray, chunk: FileChunk): Result<ByteArray> {
+ private fun decompressChunkToFile(
+ chunkFile: File,
+ chunk: FileChunk,
</file context>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt (1)
1033-1069: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winChunk-usage sort doesn’t affect download ordering. In
EpicDownloadManager.kt:1033-1069,sortedFilesonly changes pre-allocation;networkChunkFlow.emit(...)still iterateschunkQueue, which is built earlier frombuildOrderedChunkQueue(fileChunkIds)and keeps file-appearance order. If the goal is to prioritize less-shared chunks, the emit loop needs to followsortedFiles(like GOG), otherwise the sort can be removed.🤖 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/main/java/app/gamenative/service/epic/EpicDownloadManager.kt` around lines 1033 - 1069, The chunk-usage sort in EpicDownloadManager’s file-preallocation path only affects allocation order and not actual download order. Update the logic around sortedFiles and the networkChunkFlow.emit loop so chunk emission is driven by the less-shared-first file order instead of the earlier chunkQueue built by buildOrderedChunkQueue; if download ordering is not meant to change, remove the unused sort to avoid misleading behavior.
🤖 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.
Outside diff comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 1033-1069: The chunk-usage sort in EpicDownloadManager’s
file-preallocation path only affects allocation order and not actual download
order. Update the logic around sortedFiles and the networkChunkFlow.emit loop so
chunk emission is driven by the less-shared-first file order instead of the
earlier chunkQueue built by buildOrderedChunkQueue; if download ordering is not
meant to change, remove the unused sort to avoid misleading behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d361be5e-5c40-4248-b676-c1c33ad0be01
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/test/java/app/gamenative/service/gog/GOGDownloadManagerTest.kt (2)
474-500: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name overstates what's being verified.
decompressChunkToFile_emptyMd5_failsValidationimplies a dedicated empty-MD5 validation path, but per the production implementation this is just a regular MD5-mismatch comparison (empty string never equals a computed digest). The inline comment on line 487 already clarifies this correctly — consider renaming to something likedecompressChunkToFile_emptyMd5_failsAsMismatchfor accuracy.🤖 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/service/gog/GOGDownloadManagerTest.kt` around lines 474 - 500, The test name in GOGDownloadManagerTest overstates a special empty-MD5 validation path; this case is handled by the normal MD5 comparison in testDecompressChunkToFile, where an empty string simply fails as a mismatch. Rename decompressChunkToFile_emptyMd5_failsValidation to reflect that it is verifying a standard MD5 mismatch (for example, an empty MD5 failing as mismatch) and keep the existing assertions aligned with that behavior.
317-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
TemporaryFolderrule instead of manual temp-dir cleanup in each test.Each of the 7 new tests creates its own
Files.createTempDirectory(...)and only callsdeleteRecursively()at the very end of the test body. If any assertion fails partway through, the temp directory (and any chunk/output files) leaks and is never cleaned up, and the same setup/teardown boilerplate is repeated 7 times.♻️ Suggested refactor using JUnit's `TemporaryFolder` rule
+ `@get`:Rule + val tempFolder = TemporaryFolder() + `@Before` fun setUp() { ... }Then in each test, replace:
- val tempDir = Files.createTempDirectory("gog-chunk-test").toFile() + val tempDir = tempFolder.newFolder("gog-chunk-test")and drop the trailing
tempDir.deleteRecursively()calls —TemporaryFoldercleans up automatically after each test, pass or fail.🤖 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/service/gog/GOGDownloadManagerTest.kt` around lines 317 - 527, The new chunk decompression tests each create temp directories manually and rely on trailing cleanup, which leaks files if an assertion fails. Refactor the tests in GOGDownloadManagerTest to use a JUnit TemporaryFolder-style setup instead of repeated Files.createTempDirectory and deleteRecursively calls, and update each decompression test to use the shared temp-folder fixture for chunk/output files so cleanup happens automatically after every test. Keep the focus on the decompression test methods and their tempDir/File setup.
🤖 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.
Nitpick comments:
In `@app/src/test/java/app/gamenative/service/gog/GOGDownloadManagerTest.kt`:
- Around line 474-500: The test name in GOGDownloadManagerTest overstates a
special empty-MD5 validation path; this case is handled by the normal MD5
comparison in testDecompressChunkToFile, where an empty string simply fails as a
mismatch. Rename decompressChunkToFile_emptyMd5_failsValidation to reflect that
it is verifying a standard MD5 mismatch (for example, an empty MD5 failing as
mismatch) and keep the existing assertions aligned with that behavior.
- Around line 317-527: The new chunk decompression tests each create temp
directories manually and rely on trailing cleanup, which leaks files if an
assertion fails. Refactor the tests in GOGDownloadManagerTest to use a JUnit
TemporaryFolder-style setup instead of repeated Files.createTempDirectory and
deleteRecursively calls, and update each decompression test to use the shared
temp-folder fixture for chunk/output files so cleanup happens automatically
after every test. Keep the focus on the decompression test methods and their
tempDir/File setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dc6367e-2dfe-4bbb-ab17-433c5d00dfd4
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/test/java/app/gamenative/service/gog/GOGDownloadManagerTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
phobos665
left a comment
phobos665
left a comment
There was a problem hiding this comment.
Looks good, thanks for improving this area.
Tests look sound too.
Refactors GOG chunk assembly to stream decompression directly to files, significantly reducing memory pressure during large downloads and parallel operations. Other improvements include: * Sorts files by chunk usage to prioritize less-shared chunks and free cache sooner. * Reduces buffer size for chunk downloads to further minimize memory. * Adds detailed disk usage logging for better monitoring. * Increases the download loop recheck delay to account for new disk IO operations.
* Sorts files by total chunk usage to prioritize less-shared chunks and free cache sooner. * Adds detailed disk usage logging for better visibility during downloads. * Increases download loop recheck delay to accommodate new disk I/O from stats calculation.
joshuatam
force-pushed
the
fix/optimize-gog-epic-download
branch
from
7806fb9 to
69f14d0
Compare
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/service/gog/GOGDownloadManager.kt`:
- Around line 1074-1075: The recheck delay in GOGDownloadManager’s stalled-chunk
loop was increased, but the stuck-chunk recovery threshold still depends on the
same attempt count, making recovery much slower. Update the logic around the
`samePendingChunksAttempts >= 10` check in `GOGDownloadManager` so the stall
detection/re-emit timing is decoupled from the `delay(5000)` cadence, keeping
the log/recheck interval at 5s if needed but triggering chunk re-emission after
fewer stalled seconds.
🪄 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
Run ID: 5351323e-1d58-44e2-8903-f52fbee3a54e
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/test/java/app/gamenative/service/gog/GOGDownloadManagerTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt
| // Wait for 5 seconds to recheck (not too quick due to added stats causing IO) | ||
| delay(5000) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Recheck delay increase also slows stuck-chunk recovery ~5×.
The samePendingChunksAttempts >= 10 re-emit threshold (Line 1061) is unchanged, so bumping the loop delay from 1,000ms to 5,000ms pushes the stuck-chunk recovery point from ~10s to ~50s. A genuinely stalled download will now appear frozen for nearly a minute before missing chunks are re-emitted. Consider decoupling the stall threshold from the log/recheck cadence (e.g., recheck at 5s but re-emit after fewer stalled seconds) so recovery stays responsive.
🤖 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/main/java/app/gamenative/service/gog/GOGDownloadManager.kt` around
lines 1074 - 1075, The recheck delay in GOGDownloadManager’s stalled-chunk loop
was increased, but the stuck-chunk recovery threshold still depends on the same
attempt count, making recovery much slower. Update the logic around the
`samePendingChunksAttempts >= 10` check in `GOGDownloadManager` so the stall
detection/re-emit timing is decoupled from the `delay(5000)` cadence, keeping
the log/recheck interval at 5s if needed but triggering chunk re-emission after
fewer stalled seconds.
3 participants
Description
For GOG:
Refactors GOG chunk assembly to stream decompression directly to files, significantly reducing memory pressure during large downloads and parallel operations.
Other improvements include:
For EPIC:
Recording
GOG logs
GPIC logs
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
Optimize GOG and Epic downloads to cut memory use and reduce I/O during large installs. GOG now streams zlib decompression directly into files with MD5 and size checks; both providers prioritize less‑shared chunks and show cache-based disk stats without scanning the install dir.
md5is missing.decompressChunkToFile(compressed/uncompressed, non-zero offsets, size/MD5 mismatch, corrupted data).calculateDirectorySize(scan cache dir only; skip symlinks; precompute expected game size); increase recheck delay to 5s.Written for commit 69f14d0. Summary will update on new commits.
Summary by CodeRabbit
Summary of Changes
New Features
Bug Fixes
Improvements
Tests