Modern external storage - #1768
Conversation
📝 WalkthroughWalkthrough
ChangesExternal storage download flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SteamService
participant DepotDownloader
participant CaseInsensitiveFileSystem
participant RedirectRoot
SteamService->>CaseInsensitiveFileSystem: configure chunk staging redirect
SteamService->>DepotDownloader: start app download
DepotDownloader->>CaseInsensitiveFileSystem: write and read depot chunks
CaseInsensitiveFileSystem->>RedirectRoot: redirect eligible chunk I/O
SteamService->>RedirectRoot: delete staging directory on completion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/data/DownloadInfo.kt (1)
113-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for external persistence.
Extend
app/src/test/java/app/gamenative/data/DownloadInfoTest.ktto cover throttling, forced persistence under contention, sibling-path classification, and failed temporary-file replacement.Also applies to: 309-315
🤖 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/data/DownloadInfo.kt` around lines 113 - 133, Extend DownloadInfoTest to cover external-storage persistence behavior: verify repeated non-forced calls are throttled while internal paths persist each time, forced persistence succeeds under concurrent contention, sibling paths are not misclassified as internal by isOnExternalStorage, and failures during temporary-file replacement are handled without corrupting the existing snapshot.app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt (1)
113-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
mustCreate/mustExistare silently ignored on the redirect path.When a chunk redirect applies,
createDirectoryalways callsdelegate.createDirectories(target)regardless ofmustCreate, anddeleteRecursivelynever checksmustExistbefore returning. Per Okio'sFileSystemcontract,mustCreate=trueshould throw if the directory already exists, andmustExist=trueshould throw if neither the target nor the original path exists. Silently swallowing these flags for chunk paths could mask a "directory already exists"/"path missing" signal that DepotDownloader relies on internally.🔧 Proposed fix to honor the contract
override fun createDirectory(dir: Path, mustCreate: Boolean) { if (chunkRedirectTarget(dir) != null) { val target = chunkRedirectForWrite(dir) ?: dir + if (mustCreate && delegate.metadataOrNull(target) != null) { + throw IOException("$dir already exists.") + } delegate.createDirectories(target) return } super.createDirectory(dir, mustCreate) } override fun deleteRecursively(fileOrDirectory: Path, mustExist: Boolean) { val target = chunkRedirectTarget(fileOrDirectory) if (target != null) { - if (delegate.metadataOrNull(target) != null) { + val targetExists = delegate.metadataOrNull(target) != null + val originalExists = delegate.metadataOrNull(fileOrDirectory) != null + if (mustExist && !targetExists && !originalExists) { + throw IOException("$fileOrDirectory does not exist.") + } + if (targetExists) { delegate.deleteRecursively(target) } - if (delegate.metadataOrNull(fileOrDirectory) != null) { + if (originalExists) { delegate.deleteRecursively(fileOrDirectory) } return } super.deleteRecursively(fileOrDirectory, mustExist) }🤖 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/utils/CaseInsensitiveFileSystem.kt` around lines 113 - 134, Update the redirect branches in createDirectory and deleteRecursively to honor their mustCreate and mustExist parameters. In createDirectory, preserve the existing-target check and throw the same already-exists error required by the FileSystem contract when mustCreate is true, otherwise create the redirected target as appropriate. In deleteRecursively, track whether either the redirected target or original path exists and throw the contract-required missing-path error when mustExist is true and neither exists; retain current deletion behavior when the flags do not require an error.
🤖 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 131-134: Update isOnExternalStorage to normalize or canonicalize
both path and DownloadService.baseDataDirPath, then determine containment using
path-segment-aware path APIs instead of startsWith. Preserve the blank
internalRoot behavior while ensuring sibling paths such as files2 are classified
as external.
- Line 49: Update persistProgressSnapshot and all listed call sites so the
throttle check and snapshot file write are serialized under one shared lock.
Ensure force = true calls proceed without being rejected by another caller’s CAS
result, while preserving throttling for non-forced calls and preventing
overlapping writes to the shared .tmp file.
- Around line 309-315: Update the external-storage branch in the DownloadInfo
persistence flow to check the Boolean result of tmp.renameTo(file). When the
rename fails, delete the temporary file and explicitly handle the failure by
throwing or catching and logging it, or replace the operation with an API that
reports replacement errors; preserve the direct write path for non-external
storage.
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 1846-1849: Make chunkStagingRedirectDir unique for each download
attempt instead of deriving it solely from appId, using an attempt-specific
identifier while preserving the existing external-storage conditional behavior.
Update all related staging-directory usage and cleanup in downloadApp, including
the exception handler and invokeOnCompletion paths, so each job deletes only its
own directory.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/data/DownloadInfo.kt`:
- Around line 113-133: Extend DownloadInfoTest to cover external-storage
persistence behavior: verify repeated non-forced calls are throttled while
internal paths persist each time, forced persistence succeeds under concurrent
contention, sibling paths are not misclassified as internal by
isOnExternalStorage, and failures during temporary-file replacement are handled
without corrupting the existing snapshot.
In `@app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt`:
- Around line 113-134: Update the redirect branches in createDirectory and
deleteRecursively to honor their mustCreate and mustExist parameters. In
createDirectory, preserve the existing-target check and throw the same
already-exists error required by the FileSystem contract when mustCreate is
true, otherwise create the redirected target as appropriate. In
deleteRecursively, track whether either the redirected target or original path
exists and throw the contract-required missing-path error when mustExist is true
and neither exists; retain current deletion behavior when the flags do not
require an error.
🪄 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: b401c7f3-705f-475b-a436-e6988b3a43f0
📒 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/utils/CaseInsensitiveFileSystem.ktapp/src/main/res/values/strings.xmlapp/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt
| fun cancel(message: String) { | ||
| // Persist the most recent progress so a resume can pick up where it left off. | ||
| persistProgressSnapshot() | ||
| persistProgressSnapshot(force = true) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize persistence calls so forced snapshots cannot be dropped.
A concurrent caller can update lastPersistMs between get() and compareAndSet(), causing a force = true cancellation/failure snapshot to return without writing. Calls can also overlap while sharing the same .tmp file, producing stale or out-of-order snapshots. Serialize the throttle decision and the actual file write with one lock; forced calls should not fail merely because another caller won the CAS.
Also applies to: 120-128, 292-295, 309-312
🤖 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/data/DownloadInfo.kt` at line 49, Update
persistProgressSnapshot and all listed call sites so the throttle check and
snapshot file write are serialized under one shared lock. Ensure force = true
calls proceed without being rejected by another caller’s CAS result, while
preserving throttling for non-forced calls and preventing overlapping writes to
the shared .tmp file.
| private fun isOnExternalStorage(path: String): Boolean { | ||
| val internalRoot = DownloadService.baseDataDirPath | ||
| return internalRoot.isNotBlank() && !path.startsWith(internalRoot) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use path-segment containment for storage classification.
path.startsWith(internalRoot) treats sibling paths such as /.../files2 as internal, disabling external-volume throttling and atomic persistence for them. Normalize/canonicalize the paths and compare path segments rather than a raw string prefix.
🤖 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/data/DownloadInfo.kt` around lines 131 -
134, Update isOnExternalStorage to normalize or canonicalize both path and
DownloadService.baseDataDirPath, then determine containment using
path-segment-aware path APIs instead of startsWith. Preserve the blank
internalRoot behavior while ensuring sibling paths such as files2 are classified
as external.
| if (isOnExternalStorage(appDirPath)) { | ||
| val tmp = File(dir, "$PERSISTENCE_FILE.tmp") | ||
| tmp.writeText(bytesDownloaded.toString()) | ||
| tmp.renameTo(file) | ||
| } else { | ||
| file.writeText(bytesDownloaded.toString()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching DownloadInfo.kt:\n'
fd -a 'DownloadInfo.kt$' . || true
printf '\nRelevant lines/context:\n'
file="$(fd 'DownloadInfo.kt$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '260,340p' "$file" | nl -ba -v260
fi
printf '\nOccurrences of isOnExternalStorage and persistence save/load methods:\n'
rg -n "isOnExternalStorage|PERSISTENCE_FILE|writeText|renameTo|load|save|delete" "$file" || true
printf '\nProject API/version hints:\n'
for f in app/build.gradle.kts app/build.gradle minSdkVersion build.gradle settings.gradle settings.gradle.kts gradle/libs.versions.toml; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n "minSdk|targetSdk|compileSdk|android\.minSdk|targetSdkVersion|android\.targetSdk|compileSdkVersion|androidNamespace|composeOptions|kotlinOptions" "$f" || true
fi
doneRepository: utkarshdalal/GameNative
Length of output: 392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'DownloadInfo.kt$' . | head -n 1)"
printf 'file=%s\n' "$file"
printf '\nRelevant lines 260-340:\n'
sed -n '260,340p' "$file" | while IFS= read -r line; do
printf '%s\n' "$line"
done
printf '\nOccurrences:\n'
rg -n "isOnExternalStorage|PERSISTENCE_FILE|writeText|renameTo|delete|Load|load|Save|save" "$file" || true
printf '\nProject API/version hints:\n'
for f in app/build.gradle.kts app/build.gradle minSdkVersion build.gradle settings.gradle settings.gradle.kts gradle/libs.versions.toml; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n "minSdk|targetSdk|compileSdk|android\.minSdk|targetSdkVersion|android\.targetSdk|compileSdkVersion|androidNamespace|composeOptions|kotlinOptions" "$f" || true
fi
done
if command -v git >/dev/null 2>&1; then
printf '\nGit diff stat/name-status:\n'
git diff --stat || true
git diff --numstat || true
fiRepository: utkarshdalal/GameNative
Length of output: 7101
🌐 Web query:
Java File renameTo returns false documentation Android
💡 Result:
In Android, the File.renameTo(File dest) method returns false if the operation fails, and it does not throw an IOException to explain why [1][2][3]. Because the method provides no specific error information, you must always check the returned boolean value to verify success [1][4][3]. According to the official Android documentation, common reasons for failure include [1][2][3]: - Mount Point Restrictions: The source and destination paths must reside on the same mount point [1][2]. A frequent issue in Android is attempting to move files between internal storage and an SD card, which are typically separate file systems [1][5]. - Permissions: Write permission is required on the directories containing both the source and the destination files [1][2][3]. - Search Permissions: Search (execute) permission is required for all parent directories of both the source and destination paths [1][2][3]. Additional practical considerations that may cause renameTo to return false include: - The destination file already exists (depending on the implementation of the underlying file system) [4][6]. - The source file is currently open or in use by another process [6]. - Insufficient storage space [4]. - The file system is mounted as read-only or is corrupted [4]. If renameTo fails, the standard practice is to copy the source file to the destination location and then delete the original file, as this handles cases where a simple rename is not supported [5]. The Android documentation also notes that the java.nio.file.Files.move method can be used for platform-independent file moves [1][2][3].
Citations:
- 1: https://developer.android.com/reference/kotlin/java/io/File.html?authuser=1
- 2: https://developer.android.google.cn/reference/java/io/File
- 3: https://developer.android.com/reference/java/io/File
- 4: https://stackoverflow.com/questions/18369186/what-can-go-wrong-with-file-renametofile-in-android
- 5: https://stackoverflow.com/questions/4770004/how-to-move-rename-file-from-internal-app-storage-to-external-storage-on-android
- 6: https://stackoverflow.com/questions/1325388/how-to-find-out-why-renameto-failed
Handle failed renameTo results.
File.renameTo() returns false on failure without throwing. Since this path leaves the original progress file untouched on failure, the load path may return stale progress. Check the result; if it fails, delete the temp file and either throw/catch-and-log the failure or use an explicit replacement/move API for the external storage write.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 309-309: A File / FileInputStream / FileReader / FileOutputStream / FileWriter is constructed from a path built with string concatenation ("baseDir" + input) or string interpolation ("$baseDir/$input"). If any segment is attacker-controlled, a value such as "../../etc/passwd" escapes the intended directory (path traversal). Validate and canonicalize the resolved path and confirm it stays under the intended base directory (e.g. compare File(baseDir, name).canonicalFile against baseDir.canonicalFile), or reject inputs containing path separators and "..".
Context: File(dir, "$PERSISTENCE_FILE.tmp")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(path-traversal-file-concat-kotlin)
🤖 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/data/DownloadInfo.kt` around lines 309 -
315, Update the external-storage branch in the DownloadInfo persistence flow to
check the Boolean result of tmp.renameTo(file). When the rename fails, delete
the temporary file and explicitly handle the failure by throwing or catching and
logging it, or replace the operation with an API that reports replacement
errors; preserve the direct write path for non-external storage.
| // Installing to external storage: keep transient chunk files on | ||
| // internal storage so each chunk isn't written to the slow volume twice | ||
| val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId") | ||
| .takeIf { !appDirPath.startsWith(DownloadService.baseDataDirPath) } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Chunk staging dir is keyed only by appId, so a retriggered download can race the previous job's cleanup.
chunkStagingRedirectDir cleanup (deleteRecursively() at line 2131) always runs after removeDownloadJob(appId) — either the one inline in the exception handler (line 2122, unchanged) or the one in invokeOnCompletion (line 2130). Once removeDownloadJob fires, a new downloadApp() call for the same appId passes the downloadJobs.contains(appId) guard (line 1729) and can start writing fresh chunks into the very same depot_chunks/$appId directory that the old job's cleanup is about to wipe — an active sink could be deleted out from under the new attempt, causing spurious IOExceptions/failed downloads. Reordering cleanup-before-removeDownloadJob only closes the invokeOnCompletion path, not the inline exception-handler path at line 2122.
Making the staging directory unique per attempt removes the collision regardless of call ordering:
🔒 Proposed fix — make the staging dir unique per attempt
- val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId")
+ val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId-${System.nanoTime()}")
.takeIf { !appDirPath.startsWith(DownloadService.baseDataDirPath) }Also applies to: 1870-1888, 2125-2131
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 1847-1847: A File / FileInputStream / FileReader / FileOutputStream / FileWriter is constructed from a path built with string concatenation ("baseDir" + input) or string interpolation ("$baseDir/$input"). If any segment is attacker-controlled, a value such as "../../etc/passwd" escapes the intended directory (path traversal). Validate and canonicalize the resolved path and confirm it stays under the intended base directory (e.g. compare File(baseDir, name).canonicalFile against baseDir.canonicalFile), or reject inputs containing path separators and "..".
Context: File(DownloadService.baseCacheDirPath, "depot_chunks/$appId")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(path-traversal-file-concat-kotlin)
🤖 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/SteamService.kt` around lines 1846 -
1849, Make chunkStagingRedirectDir unique for each download attempt instead of
deriving it solely from appId, using an attempt-specific identifier while
preserving the existing external-storage conditional behavior. Update all
related staging-directory usage and cleanup in downloadApp, including the
exception handler and invokeOnCompletion paths, so each job deletes only its own
directory.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 5 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/CaseInsensitiveFileSystem.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt:82">
P1: A chunk rewritten after redirect space drops below 1 GiB is saved at the original path, but reads still select the stale redirected copy. Keep writing an already-redirected file to its existing location, or remove that copy before falling back.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt:107">
P2: Deleting a missing redirected chunk tree with `mustExist = true` silently succeeds, unlike every non-redirected path. Preserve the normal missing-path behavior when neither redirect nor original exists.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt:113">
P2: `createDirectory` ignores the `mustCreate` parameter for redirected chunk paths. When the path maps to the redirect volume, the method calls `delegate.createDirectories(target)` instead of `delegate.createDirectory(target, mustCreate)`. This means the caller's expectation about whether the directory should already exist is silently discarded — a call with `mustCreate=true` that expects an `IOException` on conflict will instead succeed. If the parent directories of the redirect target are guaranteed to exist, consider calling `delegate.createDirectory(target, mustCreate)` directly; otherwise use `delegate.createDirectories(target.parent)` followed by `delegate.createDirectory(target, mustCreate)`.</violation>
<violation number="4" location="app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt:122">
P2: `deleteRecursively` ignores the `mustExist` parameter for redirected chunk paths. The `mustExist` contract from the caller is not forwarded: `delegate.deleteRecursively(target)` uses the single-argument default (`true`), and if neither the redirect nor the original path exists when `mustExist=true`, the method returns silently instead of throwing the expected `IOException`. Forward `mustExist` explicitly: `delegate.deleteRecursively(target, mustExist)` (and for the original path) so the semantics are preserved, and keep the silent return only when `mustExist=false`.</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:120">
P3: This persistence behavior has no focused regression coverage. Add tests for external vs. internal paths, interval/force behavior, and a failed rename so future changes do not silently lose resume progress.
(Based on your team's feedback about tests for complex logic.) [FEEDBACK_USED]</violation>
<violation number="2" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:125">
P2: Cancellation or service teardown within two seconds of a chunk snapshot can now leave the resume file behind the downloaded data, because these terminal callers still use the throttled default. Pass `force = true` at terminal persistence sites (or otherwise distinguish terminal snapshots) so resume state is not intentionally dropped.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:126">
P2: A forced final snapshot can be dropped when another persistence callback updates `lastPersistMs` between `get()` and this CAS. Handle `force` outside the CAS gate (and serialize writes if needed) so `cancel()`/failure persistence retains its force guarantee.</violation>
<violation number="4" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:133">
P2: `path.startsWith(internalRoot)` is a raw string-prefix check. If `internalRoot` doesn't end with a path separator, a sibling directory sharing the same prefix (e.g., `<root>2/...`) would incorrectly match and be classified as internal storage, disabling external-volume throttling and atomic persistence for that path. Adding a trailing separator to the comparison (or comparing canonicalized path segments) would make this robust.</violation>
<violation number="5" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:312">
P2: `persistBytesDownloaded` ignores the return value of `tmp.renameTo(file)` for the external-storage path. If the rename fails (e.g., due to a locked file or volume limitation), the progress snapshot is silently dropped — the original file retains stale data, the temp file is abandoned, and no error surfaces. Consider checking the return value and falling back to a direct write (losing atomicity but still persisting) or logging the failure so it doesn't go unnoticed.</violation>
</file>
<file name="app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt">
<violation number="1" location="app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt:128">
P2: The test depends on > 1 GiB free space on the temp partition (via the `REDIRECT_MIN_FREE_BYTES` check in `chunkRedirectForWrite`). When free space is below threshold during CI runs, the redirect silently falls through to the original path and assertions like `File(redirectDir, "file1/0_abc.chunk").exists()` will fail, making the test flaky in constrained environments.</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:1848">
P2: The chunk staging directory is keyed only by `appId`, and `deleteRecursively()` runs *after* `removeDownloadJob(appId)` in `invokeOnCompletion`. Once the job is removed from the map, a retriggered download for the same app can start writing into the same `depot_chunks/$appId` directory that the old job's cleanup is about to wipe — potentially deleting active chunk files from the new attempt. Making the staging directory unique per attempt (e.g., appending a timestamp or attempt ID) would eliminate this race regardless of cleanup ordering.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| override fun sink(file: Path, mustCreate: Boolean): Sink { | ||
| if (chunkRedirectTarget(file) != null) { | ||
| val target = chunkRedirectForWrite(file) ?: file |
There was a problem hiding this comment.
P1: A chunk rewritten after redirect space drops below 1 GiB is saved at the original path, but reads still select the stale redirected copy. Keep writing an already-redirected file to its existing location, or remove that copy before falling back.
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/utils/CaseInsensitiveFileSystem.kt, line 82:
<comment>A chunk rewritten after redirect space drops below 1 GiB is saved at the original path, but reads still select the stale redirected copy. Keep writing an already-redirected file to its existing location, or remove that copy before falling back.</comment>
<file context>
@@ -43,6 +57,80 @@ class CaseInsensitiveFileSystem(
+
+ override fun sink(file: Path, mustCreate: Boolean): Sink {
+ if (chunkRedirectTarget(file) != null) {
+ val target = chunkRedirectForWrite(file) ?: file
+ target.parent?.let { delegate.createDirectories(it) }
+ return delegate.sink(target, mustCreate)
</file context>
| val target = chunkRedirectForWrite(file) ?: file | |
| val target = chunkRedirectTarget(file) | |
| ?.takeIf { delegate.metadataOrNull(it) != null } | |
| ?: chunkRedirectForWrite(file) | |
| ?: file |
| chunkRedirectTarget(path)?.let { target -> | ||
| if (delegate.metadataOrNull(target) != null) { | ||
| delegate.delete(target, mustExist) | ||
| return |
There was a problem hiding this comment.
P2: Deleting a missing redirected chunk tree with mustExist = true silently succeeds, unlike every non-redirected path. Preserve the normal missing-path behavior when neither redirect nor original exists.
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/utils/CaseInsensitiveFileSystem.kt, line 107:
<comment>Deleting a missing redirected chunk tree with `mustExist = true` silently succeeds, unlike every non-redirected path. Preserve the normal missing-path behavior when neither redirect nor original exists.</comment>
<file context>
@@ -43,6 +57,80 @@ class CaseInsensitiveFileSystem(
+ chunkRedirectTarget(path)?.let { target ->
+ if (delegate.metadataOrNull(target) != null) {
+ delegate.delete(target, mustExist)
+ return
+ }
+ }
</file context>
| val now = System.currentTimeMillis() | ||
| val last = lastPersistMs.get() | ||
| if (!force && now - last < PERSIST_INTERVAL_MS) return | ||
| if (!lastPersistMs.compareAndSet(last, now)) return |
There was a problem hiding this comment.
P2: A forced final snapshot can be dropped when another persistence callback updates lastPersistMs between get() and this CAS. Handle force outside the CAS gate (and serialize writes if needed) so cancel()/failure persistence retains its force guarantee.
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/data/DownloadInfo.kt, line 126:
<comment>A forced final snapshot can be dropped when another persistence callback updates `lastPersistMs` between `get()` and this CAS. Handle `force` outside the CAS gate (and serialize writes if needed) so `cancel()`/failure persistence retains its force guarantee.</comment>
<file context>
@@ -109,8 +110,27 @@ data class DownloadInfo(
+ val now = System.currentTimeMillis()
+ val last = lastPersistMs.get()
+ if (!force && now - last < PERSIST_INTERVAL_MS) return
+ if (!lastPersistMs.compareAndSet(last, now)) return
+ }
+ persistBytesDownloaded(path)
</file context>
| if (isOnExternalStorage(path)) { | ||
| val now = System.currentTimeMillis() | ||
| val last = lastPersistMs.get() | ||
| if (!force && now - last < PERSIST_INTERVAL_MS) return |
There was a problem hiding this comment.
P2: Cancellation or service teardown within two seconds of a chunk snapshot can now leave the resume file behind the downloaded data, because these terminal callers still use the throttled default. Pass force = true at terminal persistence sites (or otherwise distinguish terminal snapshots) so resume state is not intentionally dropped.
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/data/DownloadInfo.kt, line 125:
<comment>Cancellation or service teardown within two seconds of a chunk snapshot can now leave the resume file behind the downloaded data, because these terminal callers still use the throttled default. Pass `force = true` at terminal persistence sites (or otherwise distinguish terminal snapshots) so resume state is not intentionally dropped.</comment>
<file context>
@@ -109,8 +110,27 @@ data class DownloadInfo(
+ if (isOnExternalStorage(path)) {
+ val now = System.currentTimeMillis()
+ val last = lastPersistMs.get()
+ if (!force && now - last < PERSIST_INTERVAL_MS) return
+ if (!lastPersistMs.compareAndSet(last, now)) return
+ }
</file context>
| super.createDirectory(dir, mustCreate) | ||
| } | ||
|
|
||
| override fun deleteRecursively(fileOrDirectory: Path, mustExist: Boolean) { |
There was a problem hiding this comment.
P2: deleteRecursively ignores the mustExist parameter for redirected chunk paths. The mustExist contract from the caller is not forwarded: delegate.deleteRecursively(target) uses the single-argument default (true), and if neither the redirect nor the original path exists when mustExist=true, the method returns silently instead of throwing the expected IOException. Forward mustExist explicitly: delegate.deleteRecursively(target, mustExist) (and for the original path) so the semantics are preserved, and keep the silent return only when mustExist=false.
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/utils/CaseInsensitiveFileSystem.kt, line 122:
<comment>`deleteRecursively` ignores the `mustExist` parameter for redirected chunk paths. The `mustExist` contract from the caller is not forwarded: `delegate.deleteRecursively(target)` uses the single-argument default (`true`), and if neither the redirect nor the original path exists when `mustExist=true`, the method returns silently instead of throwing the expected `IOException`. Forward `mustExist` explicitly: `delegate.deleteRecursively(target, mustExist)` (and for the original path) so the semantics are preserved, and keep the silent return only when `mustExist=false`.</comment>
<file context>
@@ -43,6 +57,80 @@ class CaseInsensitiveFileSystem(
+ super.createDirectory(dir, mustCreate)
+ }
+
+ override fun deleteRecursively(fileOrDirectory: Path, mustExist: Boolean) {
+ val target = chunkRedirectTarget(fileOrDirectory)
+ if (target != null) {
</file context>
| fun `chunk staging paths are redirected and cleaned up on both sides`() { | ||
| val redirectDir = createTempDir("chunk_redirect") | ||
| try { | ||
| val redirectFs = CaseInsensitiveFileSystem( |
There was a problem hiding this comment.
P2: The test depends on > 1 GiB free space on the temp partition (via the REDIRECT_MIN_FREE_BYTES check in chunkRedirectForWrite). When free space is below threshold during CI runs, the redirect silently falls through to the original path and assertions like File(redirectDir, "file1/0_abc.chunk").exists() will fail, making the test flaky in constrained environments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt, line 128:
<comment>The test depends on > 1 GiB free space on the temp partition (via the `REDIRECT_MIN_FREE_BYTES` check in `chunkRedirectForWrite`). When free space is below threshold during CI runs, the redirect silently falls through to the original path and assertions like `File(redirectDir, "file1/0_abc.chunk").exists()` will fail, making the test flaky in constrained environments.</comment>
<file context>
@@ -120,4 +120,44 @@ class CaseInsensitiveFileSystemTest {
+ fun `chunk staging paths are redirected and cleaned up on both sides`() {
+ val redirectDir = createTempDir("chunk_redirect")
+ try {
+ val redirectFs = CaseInsensitiveFileSystem(
+ chunkStagingRedirect = redirectDir.toOkioPath(),
+ )
</file context>
| if (isOnExternalStorage(appDirPath)) { | ||
| val tmp = File(dir, "$PERSISTENCE_FILE.tmp") | ||
| tmp.writeText(bytesDownloaded.toString()) | ||
| tmp.renameTo(file) |
There was a problem hiding this comment.
P2: persistBytesDownloaded ignores the return value of tmp.renameTo(file) for the external-storage path. If the rename fails (e.g., due to a locked file or volume limitation), the progress snapshot is silently dropped — the original file retains stale data, the temp file is abandoned, and no error surfaces. Consider checking the return value and falling back to a direct write (losing atomicity but still persisting) or logging the failure so it doesn't go unnoticed.
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/data/DownloadInfo.kt, line 312:
<comment>`persistBytesDownloaded` ignores the return value of `tmp.renameTo(file)` for the external-storage path. If the rename fails (e.g., due to a locked file or volume limitation), the progress snapshot is silently dropped — the original file retains stale data, the temp file is abandoned, and no error surfaces. Consider checking the return value and falling back to a direct write (losing atomicity but still persisting) or logging the failure so it doesn't go unnoticed.</comment>
<file context>
@@ -281,7 +306,13 @@ data class DownloadInfo(
+ if (isOnExternalStorage(appDirPath)) {
+ val tmp = File(dir, "$PERSISTENCE_FILE.tmp")
+ tmp.writeText(bytesDownloaded.toString())
+ tmp.renameTo(file)
+ } else {
+ file.writeText(bytesDownloaded.toString())
</file context>
|
|
||
| private fun isOnExternalStorage(path: String): Boolean { | ||
| val internalRoot = DownloadService.baseDataDirPath | ||
| return internalRoot.isNotBlank() && !path.startsWith(internalRoot) |
There was a problem hiding this comment.
P2: path.startsWith(internalRoot) is a raw string-prefix check. If internalRoot doesn't end with a path separator, a sibling directory sharing the same prefix (e.g., <root>2/...) would incorrectly match and be classified as internal storage, disabling external-volume throttling and atomic persistence for that path. Adding a trailing separator to the comparison (or comparing canonicalized path segments) would make this robust.
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/data/DownloadInfo.kt, line 133:
<comment>`path.startsWith(internalRoot)` is a raw string-prefix check. If `internalRoot` doesn't end with a path separator, a sibling directory sharing the same prefix (e.g., `<root>2/...`) would incorrectly match and be classified as internal storage, disabling external-volume throttling and atomic persistence for that path. Adding a trailing separator to the comparison (or comparing canonicalized path segments) would make this robust.</comment>
<file context>
@@ -109,8 +110,27 @@ data class DownloadInfo(
+
+ private fun isOnExternalStorage(path: String): Boolean {
+ val internalRoot = DownloadService.baseDataDirPath
+ return internalRoot.isNotBlank() && !path.startsWith(internalRoot)
}
</file context>
| return internalRoot.isNotBlank() && !path.startsWith(internalRoot) | |
| val normalizedRoot = if (internalRoot.endsWith(File.separator)) internalRoot else internalRoot + File.separator | |
| return internalRoot.isNotBlank() && !path.startsWith(normalizedRoot) && path != internalRoot |
|
|
||
| // Installing to external storage: keep transient chunk files on | ||
| // internal storage so each chunk isn't written to the slow volume twice | ||
| val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId") |
There was a problem hiding this comment.
P2: The chunk staging directory is keyed only by appId, and deleteRecursively() runs after removeDownloadJob(appId) in invokeOnCompletion. Once the job is removed from the map, a retriggered download for the same app can start writing into the same depot_chunks/$appId directory that the old job's cleanup is about to wipe — potentially deleting active chunk files from the new attempt. Making the staging directory unique per attempt (e.g., appending a timestamp or attempt ID) would eliminate this race regardless of cleanup ordering.
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/SteamService.kt, line 1848:
<comment>The chunk staging directory is keyed only by `appId`, and `deleteRecursively()` runs *after* `removeDownloadJob(appId)` in `invokeOnCompletion`. Once the job is removed from the map, a retriggered download for the same app can start writing into the same `depot_chunks/$appId` directory that the old job's cleanup is about to wipe — potentially deleting active chunk files from the new attempt. Making the staging directory unique per attempt (e.g., appending a timestamp or attempt ID) would eliminate this race regardless of cleanup ordering.</comment>
<file context>
@@ -1841,9 +1837,17 @@ class SteamService : Service(), IChallengeUrlChanged {
+ // Installing to external storage: keep transient chunk files on
+ // internal storage so each chunk isn't written to the slow volume twice
+ val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId")
+ .takeIf { !appDirPath.startsWith(DownloadService.baseDataDirPath) }
+
</file context>
| val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId") | |
| val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/${appId}_${System.nanoTime()}") |
| * synchronous writes, saturating FUSE badly enough to ANR the whole app. | ||
| * Internal-storage downloads keep the original persist-every-call behavior. | ||
| */ | ||
| fun persistProgressSnapshot(force: Boolean = false) { |
There was a problem hiding this comment.
P3: This persistence behavior has no focused regression coverage. Add tests for external vs. internal paths, interval/force behavior, and a failed rename so future changes do not silently lose resume progress.
(Based on your team's feedback about tests for complex logic.)
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/data/DownloadInfo.kt, line 120:
<comment>This persistence behavior has no focused regression coverage. Add tests for external vs. internal paths, interval/force behavior, and a failed rename so future changes do not silently lose resume progress.
(Based on your team's feedback about tests for complex logic.) </comment>
<file context>
@@ -109,8 +110,27 @@ data class DownloadInfo(
+ * synchronous writes, saturating FUSE badly enough to ANR the whole app.
+ * Internal-storage downloads keep the original persist-every-call behavior.
+ */
+ fun persistProgressSnapshot(force: Boolean = false) {
+ val path = persistencePath ?: return
+ if (isOnExternalStorage(path)) {
</file context>
e77ad4a to
123f8c7
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/SteamService.kt`:
- Around line 1835-1837: Update the containment check in the
chunkStagingRedirectDir expression to normalize the base data directory and
appDirPath, then compare them using directory-aware Path.startsWith semantics
rather than raw String.startsWith. Preserve the existing takeIf behavior so
redirection is disabled only when appDirPath is actually within the base data
directory.
🪄 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: fff57c7c-9a1a-4cce-99b2-780e9f1b08f2
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.ktapp/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt
- app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt
| val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId") | ||
| .takeIf { !appDirPath.startsWith(DownloadService.baseDataDirPath) } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use directory-aware path containment.
startsWith compares raw characters, so a sibling such as /data/app2/... is treated as being under /data/app/.... That can disable the redirect and stage DepotDownloader chunks on external storage. Normalize both paths and use Path.startsWith (or canonical containment) instead.
Proposed fix
val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId")
- .takeIf { !appDirPath.startsWith(DownloadService.baseDataDirPath) }
+ .takeIf {
+ val appPath = Paths.get(appDirPath).toAbsolutePath().normalize()
+ val internalPath = Paths.get(DownloadService.baseDataDirPath)
+ .toAbsolutePath().normalize()
+ !appPath.startsWith(internalPath)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId") | |
| .takeIf { !appDirPath.startsWith(DownloadService.baseDataDirPath) } | |
| val chunkStagingRedirectDir = File(DownloadService.baseCacheDirPath, "depot_chunks/$appId") | |
| .takeIf { | |
| val appPath = Paths.get(appDirPath).toAbsolutePath().normalize() | |
| val internalPath = Paths.get(DownloadService.baseDataDirPath) | |
| .toAbsolutePath().normalize() | |
| !appPath.startsWith(internalPath) | |
| } |
🤖 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/SteamService.kt` around lines 1835 -
1837, Update the containment check in the chunkStagingRedirectDir expression to
normalize the base data directory and appDirPath, then compare them using
directory-aware Path.startsWith semantics rather than raw String.startsWith.
Preserve the existing takeIf behavior so redirection is disabled only when
appDirPath is actually within the base data directory.
…ilds The modern override forced the external install root to the primary emulated volume (internal flash). App-scoped Android/data dirs on SD/USB volumes are equally permission-free and are all the settings picker ever stores, so use the picked volume like legacy and the other stores already do.
…nstalls Each chunk was written to the install volume twice (compressed temp, then decompressed final) plus a create/delete pair, roughly halving install throughput on SD cards. Redirect the transient chunk dir to the internal cache dir via the injected filesystem, with low-space fallback to the old location, and wipe it when the download job ends.
123f8c7 to
aea9cce
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/SteamService.kt`:
- Around line 452-453: Update getAppDirPath() to select externalAppInstallPath
only when both PrefManager.useExternalStorage and externalStorageReady are true;
otherwise fall back to the internal install path so downloadApp() never persists
an unavailable or blank external location.
🪄 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: ecc6ea73-5a16-430e-bb30-51ce5b611296
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.ktapp/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt
- app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt
| private val externalAppInstallRoot: String | ||
| get() = if (BuildConfig.MODERN_ANDROID && DownloadService.baseExternalAppDirPath.isNotBlank()) { | ||
| DownloadService.baseExternalAppDirPath + "/files" | ||
| } else { | ||
| PrefManager.externalStoragePath | ||
| } | ||
| get() = PrefManager.externalStoragePath |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use external-storage readiness for install-path selection.
getAppDirPath() still selects externalAppInstallPath whenever PrefManager.useExternalStorage is true, even when externalStorageReady is false because the volume is unavailable or blank. downloadApp() then persists and uses that invalid external path instead of falling back to internal storage. Gate the install-path decision on externalStorageReady.
🤖 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/SteamService.kt` around lines 452 -
453, Update getAppDirPath() to select externalAppInstallPath only when both
PrefManager.useExternalStorage and externalStorageReady are true; otherwise fall
back to the internal install path so downloadApp() never persists an unavailable
or blank external location.
1 participant
Description
Make external storage work on the modern build
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
Fixes external installs on modern Android by using the user-selected app-scoped volume. Speeds up SD/USB installs by staging depot chunk temps in the internal cache during external installs.
Bug Fixes
.DepotDownloader/staging/chunksto internal cache viaCaseInsensitiveFileSystemwith low-space fallback; create and clean per-app staging dirs at start/end.DownloadInfo(2s) and force-save on cancel/error; use temp+rename for resilience.New Features
Written for commit aea9cce. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests