feat: streaming download+assembly for Epic/GOG to reduce disk usage#1092
Open
jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
Open
feat: streaming download+assembly for Epic/GOG to reduce disk usage#1092jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughA streaming download-and-assembly pipeline replaces two-phase chunk downloading followed by file assembly across Epic and GOG download managers. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Downloader as Download Manager
participant Cache as Chunk Cache
participant Assembler as File Assembler
participant Deleter as Chunk Deleter
Client->>Downloader: Start streaming download+assembly
Downloader->>Downloader: buildOrderedChunkQueue(fileChunkIds)
Downloader->>Downloader: buildChunkLastFileMap(fileChunkIds)
loop for each chunk batch
Downloader->>Cache: Download chunk batch
Cache-->>Downloader: Chunks available
Downloader->>Downloader: Update downloadedChunkIds
loop while consecutive files ready
Downloader->>Assembler: Assemble next ready file
Assembler-->>Downloader: File assembled
Downloader->>Deleter: chunksToDelete(..., fileIndex)
Deleter->>Cache: Delete chunk files whose lastFile == fileIndex
Cache-->>Deleter: Deleted
Downloader->>Downloader: Increment assembled count / progress
end
end
Downloader->>Client: Complete (or fail if assembly incomplete)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 897-906: The code currently only checks cancellation between chunk
batches, so make the chunk download coroutines cooperative: inside
downloadChunkWithRetry(...) and the underlying downloadChunk(...), periodically
check downloadInfo.isActive() (or use coroutineContext.isActive/ensureActive)
before and after network or disk IO and throw a CancellationException when
inactive, and ensure the async calls are launched in a coroutineScope tied to
the parent so children are cancelled when the parent is cancelled; update
downloadChunkWithRetry and downloadChunk to bail quickly on cancellation and
propagate that exception so awaitAll() doesn't wait for full completion.
- Around line 234-239: The DLC download loop currently swallows failures from
downloadAndAssembleEpicChunks by logging and continuing; change it to propagate
failures the same way as the base-game path: when
downloadAndAssembleEpicChunks(...) returns downloadResult and
downloadResult.isFailure is true, immediately return@withContext downloadResult
(instead of logging and continuing). Update the DLC loop handling that calls
downloadAndAssembleEpicChunks with variables chunkQueue, files, chunkLastFile,
chunkCacheDir, chunkDir, cdnUrls, installDir, downloadInfo so
cancellations/failures abort the install consistently.
- Around line 895-940: The progress reset at the start of the chunk loop
(downloadInfo.setProgress(0.0f)) causes per-manifest progress to drop when DLCs
are processed; remove that reset and compute progress cumulatively using bytes
(or sizes) instead of chunk counts: track bytesDownloadedThisManifest from
downloadedChunkIds (or sum chunk.size while processing chunkQueue/when
downloadChunkWithRetry completes), add any previouslyCompletedBytes stored on
downloadInfo (e.g., downloadInfo.previousDownloadedBytes or derive from
downloadInfo.totalExpectedBytes - remaining) and then call
downloadInfo.setProgress((previouslyCompletedBytes +
bytesDownloadedThisManifest) / downloadInfo.totalExpectedBytes). Update
downloadInfo.updateStatusMessage to reflect cumulative numbers if desired;
reference symbols: downloadInfo.setProgress, downloadInfo.totalExpectedBytes,
downloadedChunkIds, downloadChunkWithRetry, chunkQueue, MAX_PARALLEL_DOWNLOADS.
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 391-400: The progress bug is caused by reusing the same
DownloadInfo (downloadInfo) and its bytes counters/totalExpectedBytes (which is
initialized from gameFiles only) for both main game and dependency chunk
downloads; dependency downloads keep incrementing bytesDownloaded and push
progress past 100%. Fix by isolating progress accounting: either compute
totalExpectedBytes to include dependency file sizes before calling
downloadAndAssembleChunks for dependencies, or create a fresh DownloadInfo (or
reset bytesDownloaded/totalExpectedBytes) for dependency-phase calls so
dependency bytes don’t get added to the main game budget. Apply the same fix
where downloadAndAssembleChunks is invoked for dependencies (also at the other
occurrence referenced around lines 985-994).
- Around line 984-997: The dependency download branch currently logs failures
from downloadAndAssembleChunks (depotResult) and continues, which swallows user
cancellations; instead detect cancellation and propagate it: after calling
downloadAndAssembleChunks (symbol: downloadAndAssembleChunks) inspect
depotResult.exceptionOrNull() for a CancellationException (or use Kotlin's
CancellationException type) and rethrow it (or return a failed Result from the
enclosing function) rather than logging and continuing, leaving the existing
logging/continue behavior only for non-cancellation errors so that
finalizeInstallSuccess() cannot run after a user-cancelled dependency download.
🪄 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: b1eddcb6-7ab0-4315-b60c-63bd425fc17d
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/service/StreamingAssembly.ktapp/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/test/java/app/gamenative/service/StreamingAssemblyTest.kt
replace two-phase (download all chunks → assemble all files) with a unified loop that assembles files front-to-back as their chunks land and deletes consumed chunks immediately. peak disk usage drops from ~2x install size to ~1x. - StreamingAssembly: shared pure logic for chunk ordering, last-file tracking, readiness checks, and safe deletion decisions - EpicDownloadManager: downloadAndAssembleEpicChunks replaces separate download+assembly phases for both base game and DLC - GOGDownloadManager: downloadAndAssembleChunks with secure link refresh, used by both main game and dependency downloads. removed dead downloadChunksSimple and assembleFiles. - 14 unit tests covering ordering, deduplication, shared chunks, cleanup safety, and full batch-loop simulations
jeremybernstein
force-pushed
the
jb/streaming-assembly
branch
from
0c74412 to
2329f05
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt (1)
929-933: Consider logging when chunks are deleted for debugging.For troubleshooting streaming assembly issues, it might be helpful to log chunk deletions at debug level. This is optional.
💡 Optional: Add debug logging for chunk cleanup
for (part in file.chunkParts) { if (chunkLastFile[part.guidStr] == nextFileToAssemble) { File(chunkCacheDir, part.guidStr).delete() + Timber.tag("Epic").d("Deleted chunk ${part.guidStr} (last consumer: file $nextFileToAssemble)") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt` around lines 929 - 933, Add a debug log when deleting chunk files in the EpicDownloadManager's cleanup loop so you can trace removals: inside the loop that iterates file.chunkParts and checks chunkLastFile[part.guidStr] == nextFileToAssemble, call the class logger at debug level with the part.guidStr, nextFileToAssemble (and any file identifier like file.id or file.name) and the chunkCacheDir before deleting File(chunkCacheDir, part.guidStr). Ensure you use the existing logger instance used elsewhere in EpicDownloadManager to keep logging consistent.app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt (1)
733-734: Minor:secureLinkContextparameter is nullable but doc comment doesn't mention it.The function signature shows
secureLinkContext: SecureLinkContext?but the behavior difference (skip refresh when null) isn't documented. Consider adding a note.💡 Optional: Document nullable secureLinkContext behavior
Add to the function's KDoc or inline comment:
+ // When secureLinkContext is null (e.g., dependency downloads), expired-link + // refresh is skipped and the original failure is returned. private suspend fun downloadAndAssembleChunks(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt` around lines 733 - 734, Update the KDoc for the function that accepts the parameter secureLinkContext: SecureLinkContext? (the method in GOGDownloadManager with parameters secureLinkContext and chunkToProductMap) to explicitly state that secureLinkContext is nullable and that when it is null the function will skip any secure link refresh behavior (i.e., no refresh is performed), and document any consequences for callers; reference the secureLinkContext parameter name and the chunkToProductMap parameter in the comment so the behavior is clear to readers.app/src/main/java/app/gamenative/service/StreamingAssembly.kt (1)
28-29: Comments indicate "used by tests" but functions are also used in production.
countReadyFilesandchunksToDeleteare marked as "used by tests" but the Epic and GOG download managers also rely on this logic (inline implementations mirroring these helpers). If these are truly test-only, consider making theminternalor moving to the test source set. If they're intended for production use, update the comments.Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/StreamingAssembly.kt` around lines 28 - 29, The comment "used by tests" on functions countReadyFiles and chunksToDelete is misleading because other production code (Epic and GOG download managers) depends on them; either make them truly test-only by moving them to the test source set or change their visibility to internal, or mark them explicitly as production APIs by removing/rewriting the comment. Inspect usages of countReadyFiles and chunksToDelete (including references from the Epic/GOG download manager implementations) and then: if only tests call them move to tests or make them internal; otherwise update the comment to reflect production usage and keep them public. Ensure the chosen approach is applied to both countReadyFiles and chunksToDelete.app/src/test/java/app/gamenative/service/StreamingAssemblyTest.kt (1)
74-101: Consider adding a test forcountReadyFileswith negativenextFileIndex.The production code has
require(nextFileIndex >= 0)validation. A test verifying this throwsIllegalArgumentExceptionwould document the contract.💡 Optional: Test negative nextFileIndex throws
`@Test`(expected = IllegalArgumentException::class) fun `countReadyFiles throws on negative nextFileIndex`() { val files = listOf(listOf("a")) StreamingAssembly.countReadyFiles(files, setOf("a"), -1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/service/StreamingAssemblyTest.kt` around lines 74 - 101, Add a unit test that asserts StreamingAssembly.countReadyFiles throws an IllegalArgumentException when called with a negative nextFileIndex to cover the production validation (require(nextFileIndex >= 0)); create a test method (e.g. using `@Test`(expected = IllegalArgumentException::class) or assertFailsWith) that calls StreamingAssembly.countReadyFiles with a simple files list and downloaded set and nextFileIndex = -1 to verify the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 929-933: Add a debug log when deleting chunk files in the
EpicDownloadManager's cleanup loop so you can trace removals: inside the loop
that iterates file.chunkParts and checks chunkLastFile[part.guidStr] ==
nextFileToAssemble, call the class logger at debug level with the part.guidStr,
nextFileToAssemble (and any file identifier like file.id or file.name) and the
chunkCacheDir before deleting File(chunkCacheDir, part.guidStr). Ensure you use
the existing logger instance used elsewhere in EpicDownloadManager to keep
logging consistent.
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 733-734: Update the KDoc for the function that accepts the
parameter secureLinkContext: SecureLinkContext? (the method in
GOGDownloadManager with parameters secureLinkContext and chunkToProductMap) to
explicitly state that secureLinkContext is nullable and that when it is null the
function will skip any secure link refresh behavior (i.e., no refresh is
performed), and document any consequences for callers; reference the
secureLinkContext parameter name and the chunkToProductMap parameter in the
comment so the behavior is clear to readers.
In `@app/src/main/java/app/gamenative/service/StreamingAssembly.kt`:
- Around line 28-29: The comment "used by tests" on functions countReadyFiles
and chunksToDelete is misleading because other production code (Epic and GOG
download managers) depends on them; either make them truly test-only by moving
them to the test source set or change their visibility to internal, or mark them
explicitly as production APIs by removing/rewriting the comment. Inspect usages
of countReadyFiles and chunksToDelete (including references from the Epic/GOG
download manager implementations) and then: if only tests call them move to
tests or make them internal; otherwise update the comment to reflect production
usage and keep them public. Ensure the chosen approach is applied to both
countReadyFiles and chunksToDelete.
In `@app/src/test/java/app/gamenative/service/StreamingAssemblyTest.kt`:
- Around line 74-101: Add a unit test that asserts
StreamingAssembly.countReadyFiles throws an IllegalArgumentException when called
with a negative nextFileIndex to cover the production validation
(require(nextFileIndex >= 0)); create a test method (e.g. using `@Test`(expected =
IllegalArgumentException::class) or assertFailsWith) that calls
StreamingAssembly.countReadyFiles with a simple files list and downloaded set
and nextFileIndex = -1 to verify the contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fb0aee3-ecf0-4b90-880d-5b274f3d42a7
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/service/StreamingAssembly.ktapp/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/test/java/app/gamenative/service/StreamingAssemblyTest.kt
Contributor
Author
|
@coderabbitai not addressing these nitpicks, I think it's fine as-is |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Owner
|
Thanks good change. I'll get it into 0.9.1 |
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
replace two-phase (download all chunks → assemble all files) with a unified loop that assembles files front-to-back as their chunks land and deletes consumed chunks immediately. peak disk usage drops from ~2x install size to ~1x.
counterproposal to #1070. closes #1091. tested with GOG Cursed to Golf (698MB compressed, 160 chunks, 50 files) — peak cache ~227MB vs 698MB without streaming.
Recording
n/a — plumbing change with no visible UI difference
Test plan
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.