fix: prevent OOM when viewing large crash/wine logs#605
fix: prevent OOM when viewing large crash/wine logs#605utkarshdalal merged 1 commit intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughReplaces synchronous log reads with asynchronous tail reads using produceState and Dispatchers.IO for crash and Wine debug logs, adds a MAX_LOG_DISPLAY_BYTES constant, and introduces a private readTail(file) helper with error handling to avoid UI blocking. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Settings UI"
participant Producer as "produceState Coroutine"
participant FS as "File System (log file)"
UI->>Producer: request log tail (visible)
Producer->>FS: readTail(file) on Dispatchers.IO
FS-->>Producer: file tail or error
Producer-->>UI: emit produced tail string
UI->>UI: open Crash/Wine dialog with produced content
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt (2)
309-343: The two-tier fallback (banner → icon → text) is well structured.The flow gracefully degrades: if the banner image fails and an icon URL is available, it shows the icon on a solid background; if the icon also fails, it falls back to text. The
iconFallbackFailedguard prevents retries and thematchParentSize()overlay is appropriate.One minor observation: when the banner fails before
iconUrlresolves (initiallynull),hideTextis set tofalseimmediately. WheniconUrllater resolves andSideEffectre-fires,showIconFallbackbecomestruebuthideTextremainsfalse, so both the icon overlay and theGameInfoBlocktext will render simultaneously. If that's unintended, you could sethideText = truewhenshowIconFallbackis set:Optional: hide text when icon fallback activates
onFailure = { if (!iconUrl.isNullOrEmpty() && !iconFallbackFailed) { showIconFallback = true + hideText = true } else { hideText = false } alpha = 0.1f },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt` around lines 309 - 343, When the banner onFailure sets showIconFallback = true (and later when showIconFallback is set by SideEffect), also set hideText = true so the GameInfoBlock text doesn't render simultaneously with the icon overlay; update the banner failure handler and the place where showIconFallback is toggled (references: showIconFallback, hideText, iconFallbackFailed, ListItemImage onFailure) to ensure hideText is true whenever showIconFallback becomes true, and restore hideText = false only when iconFallbackFailed is true or the icon onFailure path runs.
215-232:findSteamGridDBImageperforms file I/O on the main thread insideremember.This local function calls
folder.listFiles()synchronously during composition. While this is pre-existing code and not introduced by this PR, it's inconsistent with the newproduceState+Dispatchers.IOpattern used foriconUrl. Consider moving this into aproduceStateorLaunchedEffectin a follow-up to keep the GRID path fully off the main thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt` around lines 215 - 232, The function findSteamGridDBImage performs synchronous file I/O (folder.listFiles) on the main thread during composition; move this logic out of the composable and into an asynchronous producer (e.g., use produceState or LaunchedEffect with Dispatchers.IO) that computes the grid image path for CustomGame apps and exposes a state value the UI observes; specifically, extract the body of findSteamGridDBImage (CustomGameScanner.getFolderPathFromAppId, java.io.File(path).listFiles, filtering by "steamgriddb_$imageType" and supported extensions, and android.net.Uri.fromFile) into that IO coroutine so the composable only reads the resulting state and does not perform file I/O on the main thread.app/src/main/java/app/gamenative/ui/util/Images.kt (1)
49-51: WrappingonFailureinSideEffectwill re-fire on every recomposition of the failure slot.Unlike the previous direct call (which also ran during composition but only conceptually "once"),
SideEffectexplicitly contracts to run after every successful recomposition. The callers inLibraryAppItemguard against infinite loops viaiconFallbackFailedand Compose state deduplication — but any future caller that mutates state insideonFailurewithout similar guards will trigger an infinite recomposition loop.Consider adding a brief KDoc on the
onFailureparameter to warn callers about this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/util/Images.kt` around lines 49 - 51, The failure slot currently wraps onFailure with SideEffect (so it will run after every successful recomposition), which can cause infinite recompositions if callers mutate state without guards; update the KDoc for the onFailure parameter (in the composable defined in Images.kt where failure = { SideEffect { onFailure() } } is used) to clearly warn that SideEffect runs after every recomposition and that callers must ensure their onFailure implementation is idempotent or guarded (e.g., use flags like iconFallbackFailed or other Compose state deduplication) to avoid triggering recomposition loops; mention LibraryAppItem as an example of correct guarding.app/src/main/java/app/gamenative/utils/CustomGameScanner.kt (1)
196-219:findCachedIconForCustomGameskips thehasContainerguard used by the other overload.In
findIconFileForCustomGame(context, appId)(Line 146), you callcm.hasContainer(appId)beforecm.getContainerById(appId). Here,getContainerByIdis called directly (Line 207). IfgetContainerByIdthrows on a missing container (rather than returning null), thecatchblock handles it — but addinghasContainerwould be more consistent and avoid relying on exception flow for a normal case.Optional: add hasContainer guard for consistency
try { val cm = ContainerManager(context) - val container = cm.getContainerById(appId) - val relExe = container?.executablePath + if (cm.hasContainer(appId)) { + val container = cm.getContainerById(appId) + val relExe = container?.executablePath - if (!relExe.isNullOrEmpty()) { - val exeFile = File(folder, relExe.replace('/', File.separatorChar)) - val outIco = File(exeFile.parentFile, exeFile.nameWithoutExtension + ".extracted.ico") - if (outIco.exists()) return outIco.absolutePath + if (!relExe.isNullOrEmpty()) { + val exeFile = File(folder, relExe.replace('/', File.separatorChar)) + val outIco = File(exeFile.parentFile, exeFile.nameWithoutExtension + ".extracted.ico") + if (outIco.exists()) return outIco.absolutePath + } } } catch (e: Exception) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/CustomGameScanner.kt` around lines 196 - 219, The findCachedIconForCustomGame function currently calls ContainerManager.getContainerById(appId) directly; instead, first check ContainerManager.hasContainer(appId) and only call getContainerById when that returns true to avoid relying on exceptions for the normal "no container" case—update the block that constructs ContainerManager(context) to call cm.hasContainer(appId) before cm.getContainerById(appId) and keep the existing try/catch for unexpected errors.app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt (1)
287-309:readTailis solid but has a minor TOCTOU window betweenfile.length()andreadFully.If the log file is truncated or rotated between the
length()call (Line 291) andreadFully(Line 299), anEOFExceptionwill be thrown. Thecatchblock on Line 307 handles this gracefully, so this isn't a bug — just noting it for awareness.One small readability nit:
(len - start).toInt()is always equal toMAX_LOG_DISPLAY_BYTES.toInt()in the truncated branch. You could use the constant directly to make the intent clearer.Optional: use constant directly for clarity
val start = maxOf(0L, len - MAX_LOG_DISPLAY_BYTES) java.io.RandomAccessFile(file, "r").use { raf -> raf.seek(start) - val bytes = ByteArray((len - start).toInt()) + val bytes = ByteArray(MAX_LOG_DISPLAY_BYTES.toInt()) raf.readFully(bytes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt` around lines 287 - 309, readTail has a TOCTOU window between len/start and raf.readFully and uses (len - start).toInt() though that value effectively equals MAX_LOG_DISPLAY_BYTES; update read logic in readTail to allocate the read buffer using MAX_LOG_DISPLAY_BYTES.toInt() (instead of (len - start).toInt()) and then bound the actual bytes to read by checking raf.length() - start (or use raf.read into a buffer and use the returned count) so you don't request more bytes than available; keep the try/catch but consider specifically handling EOFException from raf.readFully by falling back to the bytes actually read or returning the partial contents to avoid the transient rotation error.
🤖 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/ui/screen/library/components/LibraryAppItem.kt`:
- Around line 309-343: When the banner onFailure sets showIconFallback = true
(and later when showIconFallback is set by SideEffect), also set hideText = true
so the GameInfoBlock text doesn't render simultaneously with the icon overlay;
update the banner failure handler and the place where showIconFallback is
toggled (references: showIconFallback, hideText, iconFallbackFailed,
ListItemImage onFailure) to ensure hideText is true whenever showIconFallback
becomes true, and restore hideText = false only when iconFallbackFailed is true
or the icon onFailure path runs.
- Around line 215-232: The function findSteamGridDBImage performs synchronous
file I/O (folder.listFiles) on the main thread during composition; move this
logic out of the composable and into an asynchronous producer (e.g., use
produceState or LaunchedEffect with Dispatchers.IO) that computes the grid image
path for CustomGame apps and exposes a state value the UI observes;
specifically, extract the body of findSteamGridDBImage
(CustomGameScanner.getFolderPathFromAppId, java.io.File(path).listFiles,
filtering by "steamgriddb_$imageType" and supported extensions, and
android.net.Uri.fromFile) into that IO coroutine so the composable only reads
the resulting state and does not perform file I/O on the main thread.
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 287-309: readTail has a TOCTOU window between len/start and
raf.readFully and uses (len - start).toInt() though that value effectively
equals MAX_LOG_DISPLAY_BYTES; update read logic in readTail to allocate the read
buffer using MAX_LOG_DISPLAY_BYTES.toInt() (instead of (len - start).toInt())
and then bound the actual bytes to read by checking raf.length() - start (or use
raf.read into a buffer and use the returned count) so you don't request more
bytes than available; keep the try/catch but consider specifically handling
EOFException from raf.readFully by falling back to the bytes actually read or
returning the partial contents to avoid the transient rotation error.
In `@app/src/main/java/app/gamenative/ui/util/Images.kt`:
- Around line 49-51: The failure slot currently wraps onFailure with SideEffect
(so it will run after every successful recomposition), which can cause infinite
recompositions if callers mutate state without guards; update the KDoc for the
onFailure parameter (in the composable defined in Images.kt where failure = {
SideEffect { onFailure() } } is used) to clearly warn that SideEffect runs after
every recomposition and that callers must ensure their onFailure implementation
is idempotent or guarded (e.g., use flags like iconFallbackFailed or other
Compose state deduplication) to avoid triggering recomposition loops; mention
LibraryAppItem as an example of correct guarding.
In `@app/src/main/java/app/gamenative/utils/CustomGameScanner.kt`:
- Around line 196-219: The findCachedIconForCustomGame function currently calls
ContainerManager.getContainerById(appId) directly; instead, first check
ContainerManager.hasContainer(appId) and only call getContainerById when that
returns true to avoid relying on exceptions for the normal "no container"
case—update the block that constructs ContainerManager(context) to call
cm.hasContainer(appId) before cm.getContainerById(appId) and keep the existing
try/catch for unexpected errors.
jeremybernstein
force-pushed
the
jb/OOM_log
branch
from
b862dfc to
3bf94f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt (1)
288-288:check()on a compile-time constant runs on every call but can never fail.
MAX_LOG_DISPLAY_BYTESis262144L, which is trivially<= Int.MAX_VALUE. The assertion adds overhead on everyreadTailinvocation without providing any runtime safety. Replace it with a comment or a compile-timerequireequivalent (e.g., a@Suppress-annotatedvalassertion at class initialisation), or simply remove it.♻️ Suggested change
-private fun readTail(file: File?): String { - check(MAX_LOG_DISPLAY_BYTES <= Int.MAX_VALUE) { "MAX_LOG_DISPLAY_BYTES exceeds Int.MAX_VALUE" } - if (file == null || !file.exists()) return "File not found: ${file?.name ?: "null"}" +// MAX_LOG_DISPLAY_BYTES (262144) fits in Int, so the toInt() cast on ByteArray size is safe. +private fun readTail(file: File?): String { + if (file == null || !file.exists()) return "File not found: ${file?.name ?: "null"}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt` at line 288, The runtime check check(MAX_LOG_DISPLAY_BYTES <= Int.MAX_VALUE) in SettingsGroupDebug (affecting readTail calls) is redundant because MAX_LOG_DISPLAY_BYTES is a compile-time constant; remove the check and either replace it with a clarifying comment next to the MAX_LOG_DISPLAY_BYTES declaration or move a one-time assert into class/object initialization if you prefer an explicit compile-time-style assertion, ensuring you reference MAX_LOG_DISPLAY_BYTES and not leave repeated checks inside readTail.
🤖 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/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 292-305: The KB labels use integer floor division (len / 1024) so
files just over the cap appear identical to the "showing all" case; update the
string construction in the branch that builds the trimmed output (inside the
RandomAccessFile use block) to compute a rounded-up KB value for total size —
e.g. use (len + 1023) / 1024 — or alternatively display the total size in bytes,
and replace the occurrences of len / 1024 and MAX_LOG_DISPLAY_BYTES / 1024 with
the new rounded or bytes representation so the "(...KB, showing last ...KB)"
message always clearly indicates truncation when it occurred.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt`:
- Line 288: The runtime check check(MAX_LOG_DISPLAY_BYTES <= Int.MAX_VALUE) in
SettingsGroupDebug (affecting readTail calls) is redundant because
MAX_LOG_DISPLAY_BYTES is a compile-time constant; remove the check and either
replace it with a clarifying comment next to the MAX_LOG_DISPLAY_BYTES
declaration or move a one-time assert into class/object initialization if you
prefer an explicit compile-time-style assertion, ensuring you reference
MAX_LOG_DISPLAY_BYTES and not leave repeated checks inside readTail.
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt
Show resolved
Hide resolved
jeremybernstein
force-pushed
the
jb/OOM_log
branch
from
3bf94f0 to
089e434
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt (1)
284-285: Consider also annotatingreadTailwith@VisibleForTestingor adding a brief KDoc.Currently
readTailisprivateand only called from twoproduceStatelambdas, which is fine. If you ever want to unit-test the truncation logic independently (boundary at exactly 256 KB, multi-byte characters, missing files, etc.), making itinternal+@VisibleForTestingwith a short doc comment would help. Not required for this PR, just a thought.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupDebug.kt` around lines 284 - 285, The helper function readTail (currently private and used from two produceState lambdas) should be made test-accessible and documented: change its visibility to internal and annotate with `@VisibleForTesting`, and add a short KDoc noting its behavior and the MAX_LOG_DISPLAY_BYTES boundary so tests can target truncation and encoding edge cases; reference the readTail function name, the MAX_LOG_DISPLAY_BYTES constant, and the produceState call sites when making this change.
🤖 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/ui/screen/settings/SettingsGroupDebug.kt`:
- Around line 284-285: The helper function readTail (currently private and used
from two produceState lambdas) should be made test-accessible and documented:
change its visibility to internal and annotate with `@VisibleForTesting`, and add
a short KDoc noting its behavior and the MAX_LOG_DISPLAY_BYTES boundary so tests
can target truncation and encoding edge cases; reference the readTail function
name, the MAX_LOG_DISPLAY_BYTES constant, and the produceState call sites when
making this change.
I think this is out of scope (and unjustified), if we add tests, we can make this change later. @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
2 participants
Summary
produceState+Dispatchers.IORandomAccessFile.seek(), preventing OOM on large logsTest plan
Summary by cubic
Prevents OOM when opening large crash or Wine logs by reading off the main thread and showing only the last 256 KB with a clear truncation note.
Written for commit 089e434. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor