fix: kill stale wine processes before launch#1195
fix: kill stale wine processes before launch#1195xXJSONDeruloXx wants to merge 3 commits intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a pre-launch helper in XServerScreen that enumerates and hard-kills stale Wine processes via new ProcessHelper APIs, polling up to 5s to ensure termination and throwing if processes persist; the helper is invoked at the start of setupXEnvironment(...). Also adds a BackHandler in PluviaMain to block back navigation while a loading dialog is visible. Changes
Sequence DiagramsequenceDiagram
participant XServer as XServerScreen
participant Helper as ProcessHelper
participant OS as OS Process Manager
XServer->>XServer: setupXEnvironment()
XServer->>XServer: hardKillStaleWineProcessesBeforeLaunch()
XServer->>Helper: listRunningWineProcesses()
Helper->>OS: query running processes
OS-->>Helper: wine process list
Helper-->>XServer: return list
XServer->>XServer: dedupe PIDs, log if any
XServer->>Helper: killAllWineProcesses()
Helper->>OS: send SIGKILL to each PID
OS-->>Helper: termination confirmations
Helper-->>XServer: completion
XServer->>Helper: listRunningWineProcesses() (re-check)
Helper->>OS: query remaining processes
OS-->>Helper: remaining list
Helper-->>XServer: return remaining list
XServer->>XServer: if any remain -> log warning and throw IllegalStateException
XServer->>XServer: continue environment setup (if none)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/xserver/XServerScreen.kt`:
- Around line 2551-2561: The current code calls
ProcessHelper.killAllWineProcesses() then immediately re-scans via
ProcessHelper.listRunningWineProcesses() and only logs any remaining PIDs;
change this to poll until no wine PIDs remain or a short timeout elapses: after
calling ProcessHelper.killAllWineProcesses() repeatedly call
ProcessHelper.listRunningWineProcesses(), parse to ints (as in
remainingWinePids), and if non-empty sleep briefly (e.g., 100–500ms) and retry
with a total timeout (e.g., 3–10s) and exponential/backoff cap; if the set
becomes empty continue, otherwise fail the launch path (throw an exception or
return a failed Result) instead of just Timber.w, and keep the Timber.w/logging
for diagnostics when timing out.
In `@app/src/main/java/com/winlator/core/ProcessHelper.java`:
- Around line 50-53: The current killAllWineProcesses() calls killProcess(pid)
for every PID returned by listRunningWineProcesses(), but that scanner is too
broad; update listRunningWineProcesses() to only return PIDs that are owned by
this app's UID and whose executable/comm is an exact "wine" or whose filename
ends with ".exe" (no substring matches like "exe" or "wine" inside other names).
To implement: read /proc/<pid>/status or use ProcessHandle/Posix API to compare
the process UID to the current process UID, and read /proc/<pid>/comm or
/proc/<pid>/exe to check for exact name or suffix; then have
killAllWineProcesses() only call killProcess(pid) on those validated PIDs
(optionally prefer a graceful SIGTERM before SIGKILL inside killProcess()).
🪄 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: 44750659-7da4-4c1b-a6e5-31896a5d532d
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/core/ProcessHelper.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/PluviaMain.kt`:
- Around line 1100-1102: Back press is currently swallowed by BackHandler when
state.loadingDialogVisible is true and SteamService.keepAlive is false;
implement a safe cancel/timeout fallback so Back can restore normal behavior:
add a cancellable prelaunch signal (e.g., a MutableState or Job started when
state.loadingDialogVisible becomes true via LaunchedEffect or
rememberCoroutineScope) and in the BackHandler callback cancel that job and set
state.loadingDialogVisible = false (or call your prelaunch cancellation
routine), and also launch a timeout coroutine (e.g., 30s) when showing the
dialog that will clear state.loadingDialogVisible if prelaunch doesn't complete;
reference BackHandler, state.loadingDialogVisible and SteamService.keepAlive
when locating where to wire the cancel() call and timeout.
🪄 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: 0b66c21d-4342-4541-affb-a27271282272
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/PluviaMain.kt
| BackHandler(enabled = state.loadingDialogVisible && !SteamService.keepAlive) { | ||
| // TODO: Make prelaunch/loading operations cancellable so Back can exit safely. | ||
| } |
There was a problem hiding this comment.
Back press is fully swallowed during loading with no fallback.
At Line 1100, Back is intercepted; at Lines 1101-1102, the empty handler creates a dead-end if prelaunch/loading hangs. Please add either a safe cancel path or a timeout fallback that restores normal back behavior.
I can draft a small cancellable prelaunch pattern (job + cancel signal + UI fallback) if you want me to open a follow-up issue proposal.
🤖 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/PluviaMain.kt` around lines 1100 - 1102,
Back press is currently swallowed by BackHandler when state.loadingDialogVisible
is true and SteamService.keepAlive is false; implement a safe cancel/timeout
fallback so Back can restore normal behavior: add a cancellable prelaunch signal
(e.g., a MutableState or Job started when state.loadingDialogVisible becomes
true via LaunchedEffect or rememberCoroutineScope) and in the BackHandler
callback cancel that job and set state.loadingDialogVisible = false (or call
your prelaunch cancellation routine), and also launch a timeout coroutine (e.g.,
30s) when showing the dialog that will clear state.loadingDialogVisible if
prelaunch doesn't complete; reference BackHandler, state.loadingDialogVisible
and SteamService.keepAlive when locating where to wire the cancel() call and
timeout.
There was a problem hiding this comment.
That's the idea for this scope, it's effectively same in master right now yeah? Animation persists even if you can supposedly back out, procs stick and you need force quit because of it anyway. This just mitigates impatient back mash on loading screen till a separate PR does proper retreat and cleanup
There was a problem hiding this comment.
@xXJSONDeruloXx, that's a fair point — if master already reaches a force-quit situation on back-during-loading (stale procs keep the animation alive regardless), then blocking the back key here is at least an honest no-op instead of a misleading one. The BackHandler prevents the UI thrash without making the underlying problem any worse.
I'll note this for future reviews and happy to open a follow-up issue for the proper cancellable-prelaunch work whenever you're ready.
✏️ Learnings added
Learnt from: xXJSONDeruloXx
Repo: utkarshdalal/GameNative PR: 1195
File: app/src/main/java/app/gamenative/ui/PluviaMain.kt:1100-1102
Timestamp: 2026-04-13T05:42:47.571Z
Learning: In `app/src/main/java/app/gamenative/ui/PluviaMain.kt`, the `BackHandler(enabled = state.loadingDialogVisible && !SteamService.keepAlive)` with an empty body is intentionally a temporary mitigation. On master, pressing Back during the loading/prelaunch phase already leads to a broken state (animation persists, Wine procs stick, force-quit required), so swallowing Back is no worse and prevents UI thrash from impatient back-mashing. Proper cancellable prelaunch (job cancellation + state reset + timeout fallback) is deferred to a separate PR. Do not flag the empty BackHandler body as a dead-end in future reviews of this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-18T16:30:26.610Z
Learning: In `app/src/main/java/app/gamenative/ui/PluviaMain.kt`, the three sites that call `MainActivity.setPendingLaunchRequest`, toggle `shownPendingLaunchSnackbar`, and conditionally call `SnackbarManager.show` (cold-start pending branch, ExternalGameLaunch pending branch, and non-terminal SteamDisconnected branch) are intentionally kept separate — the surrounding logic differs enough that extracting a shared helper would add more complexity than it removes. Do not suggest refactoring these into a helper in future reviews.
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 171
File: app/src/main/java/app/gamenative/MainActivity.kt:283-288
Timestamp: 2025-09-25T10:56:46.337Z
Learning: In MainActivity.kt, the back key handling in dispatchKeyEvent only checks for KeyEvent.KEYCODE_BACK (not KEYCODE_BUTTON_B) because it's specifically designed to intercept the device's physical back button when a game is running, not controller input.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1136
File: app/src/main/java/app/gamenative/MainActivity.kt:147-150
Timestamp: 2026-04-07T21:37:29.365Z
Learning: In `app/src/main/java/app/gamenative/MainActivity.kt`, the `onCreate` stale-session check `if (SteamService.keepAlive && PluviaApp.xEnvironment == null)` is intentionally kept as-is. The only non-game flow that sets `keepAlive = true` with `xEnvironment == null` is the folder-picker in `ContainerConfigDialog.kt` (line ~1032); if the app is killed in that state, resetting `keepAlive = false` on the next `onCreate` is harmless. Do not flag this guard as incorrectly matching non-XServer flows in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-04-08T06:26:20.804Z
Learning: In `app/src/main/java/app/gamenative/PluviaApp.kt`, `shutdownEnvironment()` intentionally does **not** call `winHandler?.stop()`. On a task-swipe / `onDestroy` path the OS reclaims the socket naturally, so stopping `WinHandler` there is unnecessary. The explicit `winHandler?.stop()` call before `PluviaApp.shutdownEnvironment()` in `XServerScreen.exit()` covers the graceful-exit case where it actually matters. Do not flag the absence of a `WinHandler` stop inside `shutdownEnvironment()` in future reviews.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 810
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/CustomGameAppScreen.kt:391-417
Timestamp: 2026-03-18T08:52:17.299Z
Learning: In the `app/src/main/java/app/gamenative/ui/screen/library/appscreen/` AppScreen files (e.g., `BaseAppScreen.kt`, `CustomGameAppScreen.kt`), using `CoroutineScope(Dispatchers.IO).launch` inside `AppMenuOption` click handlers (such as for `FetchSteamGridDBImages` and `CreateShortcut`) is an established, intentional pattern. Do not flag this as a lifecycle-safety issue in future reviews of these files.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1136
File: app/src/main/java/app/gamenative/MainActivity.kt:273-284
Timestamp: 2026-04-07T21:36:14.886Z
Learning: In the GameNative repository (`utkarshdalal/GameNative`), `SteamService.keepAlive` is set to `true` in only one place: `ContainerConfigDialog.kt:1032` (inside a folder-picker flow). During the normal XServerScreen game-launch bootstrap (`XServerScreen.kt` lines ~1529-1680), `keepAlive` is `false` throughout. The bootstrap uses a raw Java `Executors.newSingleThreadExecutor().submit {}` (not a coroutine/LaunchedEffect), so Compose teardown does not cancel it. However, since `keepAlive` is `false` during bootstrap, a task-swipe in that window does not produce a stale `keepAlive=true` on the next launch, and therefore does not cause the "stuck on container screen" bug. Do not flag onDestroy's `if (SteamService.keepAlive)` guard as failing to cover the bootstrap-window race in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-10T11:01:21.928Z
Learning: In `app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt` (around lines 371-396), the `Loading` parental state in `SteamService.parentalControlState` intentionally maps to `PrefManager.parentalAllowedAppIds` (which can be null) rather than `emptyList()`. This fail-open behavior on the `Loading` state is by design: on first launch/login the account type (managed child vs. non-child) is unknown, so showing all apps is preferable to incorrectly blocking everything. Subsequent sessions use the PrefManager cache, so restrictions are enforced correctly after the first login. Do not flag this as a security/correctness issue in future reviews.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 811
File: app/src/main/java/app/gamenative/ui/util/WindowSize.kt:38-44
Timestamp: 2026-03-11T11:17:48.473Z
Learning: In `app/src/main/java/app/gamenative/ui/util/WindowSize.kt`, `shouldShowGamepadUI()` reads `PrefManager.showGamepadHints` as a plain (non-reactive) property. This is intentional: when the user toggles the setting in the Settings screen and navigates back, the composable recomposes and picks up the updated value. Real-time reactivity (e.g., via StateFlow/collectAsState) is not required here. Do not flag this as an issue in future reviews.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 698
File: app/src/main/java/app/gamenative/utils/LaunchSteps.kt:69-72
Timestamp: 2026-03-03T11:47:17.167Z
Learning: In the LaunchSteps orchestration (app/src/main/java/app/gamenative/utils/LaunchSteps.kt), runOnce steps are intentionally marked as done regardless of their exit status. This is because some steps like GOGScriptInterpreter can return an error while still having completed necessary work, and the step should not run again on subsequent launches.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 918
File: app/src/main/java/app/gamenative/service/SteamService.kt:3051-3058
Timestamp: 2026-03-19T10:24:08.165Z
Learning: In `app/src/main/java/app/gamenative/service/SteamService.kt`, `connectToSteam()` uses `scope.launch(Dispatchers.Default)` deliberately. This is a pre-existing, intentional dispatcher choice. Do not flag or suggest changing `Dispatchers.Default` to another dispatcher in `connectToSteam()` in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-18T16:30:26.610Z
Learning: In `app/src/main/java/app/gamenative/MainActivity.kt`, `peekPendingLaunchRequest()` is an intentional non-destructive, synchronized read of `pendingLaunchRequest`. The peek + separate consume pattern (also present at line 436) is pre-existing and accepted; the worst-case outcome of a race is an unnecessary snackbar, which is benign. Do not flag `peekPendingLaunchRequest()` as a TOCTOU race issue in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1146
File: app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:832-840
Timestamp: 2026-04-08T12:05:56.079Z
Learning: In `app/src/main/java/app/gamenative/service/SteamAutoCloud.kt`, the `isCacheCleared` branch (around lines 832-840) intentionally reads and removes `appInfo.id` from `PrefManager.pendingCloudResync` at that point — before conflict resolution — because the presence/absence of the ID determines which dialog wording to display (upgrade-specific `conflictUfsVersion` vs generic local-saves-only text). This classification must happen pre-dialog. Do not flag this as needing to be deferred until after the resync succeeds in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 965
File: app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt:262-276
Timestamp: 2026-03-22T00:52:52.382Z
Learning: In `app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt`, `saveScrollAnchor` has an intentional early-return guard:
`if (visIdx == 0 && !existingAnchor.isNullOrEmpty() && existingAnchor != appId && !state.scrollAnchorRestored) return`
This protects against a ViewModel-recreation race: a real anchor (e.g. idx=32) is saved → ViewModel recreates → ON_PAUSE fires while the grid is still at idx=0 (before scroll restoration) → without the guard the anchor would be overwritten with position 0 and lost on the next recreation. Once `scrollAnchorRestored` becomes `true` (after the composable confirms the scroll completed), the guard is inactive and position 0 is a legitimate value to persist. Do not flag this guard or suggest clearing the anchor on `visIdx==0` in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1092
File: app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt:897-906
Timestamp: 2026-04-03T12:55:12.164Z
Learning: In `app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`, the cancellation check (`downloadInfo.isActive()`) is only observed between chunk batches in `downloadAndAssembleEpicChunks`. The `downloadChunkWithRetry` and `downloadChunk` functions do not check for cancellation internally. This is a pre-existing pattern, intentional and out of scope for new feature PRs. Do not flag mid-batch cancellation latency as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 918
File: app/src/main/java/app/gamenative/service/SteamService.kt:3091-3102
Timestamp: 2026-03-19T10:03:51.598Z
Learning: In `app/src/main/java/app/gamenative/service/SteamService.kt`, `connectToSteam()` uses a `retryAttemptAtStart` snapshot to guard the 5s watchdog: after the delay, the watchdog only marks the endpoint bad and disconnects if `!isConnected && retryAttempt == retryAttemptAtStart`. Since `onConnected` resets `retryAttempt = 0` and `onDisconnected` increments `retryAttempt++`, any callback activity during the 5s window changes `retryAttempt`, causing the watchdog to back off. This is the intentional, correct design. Do not flag the `delay(5000)` + `retryAttempt` guard pattern as a race condition in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 801
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:174-188
Timestamp: 2026-03-10T22:12:03.164Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`, the `applyConfigForContainer` function calls `installMissingComponentsForConfig` before `BestConfigService.parseConfigToContainerData`. This install-before-parse ordering is pre-existing behavior, not introduced by any recent PR. Do not flag this ordering as an issue in future reviews of this file.
Learnt from: xXJSONDeruloXx
Repo: utkarshdalal/GameNative PR: 1195
File: app/src/main/java/com/winlator/core/ProcessHelper.java:50-53
Timestamp: 2026-04-13T00:51:50.454Z
Learning: In `app/src/main/java/com/winlator/core/ProcessHelper.java`, `killAllWineProcesses()` deliberately uses the broad `listRunningWineProcesses()` matcher (substring "wine" or "exe" in /proc/<pid>/stat). This is intentionally a "hammer" approach called at the top of `setupXEnvironment()` before any new Wine processes start. It is safe on Android because `Process.sendSignal()` is subject to UID-based kernel enforcement — the app can only kill its own sandbox UID processes — and within that UID the only native procs are Wine-related. Do not flag this broad matcher as a cross-process safety risk in future reviews.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 698
File: app/src/main/java/app/gamenative/utils/LaunchSteps.kt:134-142
Timestamp: 2026-03-03T11:45:46.529Z
Learning: In the LaunchSteps orchestration (app/src/main/java/app/gamenative/utils/LaunchSteps.kt), the `wineserver -k` command in the runLauncher function is necessary to prevent the launcher from closing after each chained step. Without it, the launcher terminates prematurely between steps.
Learnt from: phobos665
Repo: utkarshdalal/GameNative PR: 1072
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:1309-1315
Timestamp: 2026-04-02T07:43:14.973Z
Learning: In the GameNative repository, when logging/telemetry includes Wine prefix/container paths that contain the hardcoded Wine username `xuser`, do not treat that as a PII/security issue (it is not a real user identity and contains no personally identifiable information). Only flag PII concerns for other user-identifying data not limited to the `xuser` hardcoded Wine username in prefix paths.
Learnt from: xXJSONDeruloXx
Repo: utkarshdalal/GameNative PR: 1076
File: app/src/main/java/app/gamenative/service/gog/GOGCometManager.kt:57-61
Timestamp: 2026-04-06T13:16:11.272Z
Learning: In the GameNative repository, GOG Wine containers follow a one-game-per-container invariant. Because a single container is always dedicated to exactly one GOG game, there is no risk of concurrent writes to a fixed per-container batch file (e.g., `launch_gog_with_comet.bat` in `GOGCometManager.kt`). Do not flag the reuse of a fixed launcher batch file path per container as a race-condition risk in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 581
File: app/src/main/java/app/gamenative/ui/PluviaMain.kt:1154-1176
Timestamp: 2026-02-21T22:26:41.871Z
Learning: In Kotlin coroutines, suspend functions automatically suspend and resume within a coroutine scope. Only Deferred objects produced by async { ... } require .await() to obtain the result. Do not call .await() on direct suspend function calls or on non-Deferred results; use await() only when you have a Deferred that represents a value you need.
There was a problem hiding this comment.
in some situations, installing mono runs indefinitely. This was happening on some box64/fex versions. I guess users can force stop or swipe the app away if that happens?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@utkarshdalal yeah swipe away like they do now, just with this pr they can now get a clean slate when re entering. Back button still wouldn't dismiss loading animation for boot but would retreat ui in the background. This keeps ux same but unintended soft lock till force kill is solved
Description
fixes the case where users swipe app away while a game is running, then relaunch the app to have infinite booting screen because a stray wine proc is running. This acts as a hammer to see if any wine pros exist on a game launch, and clear them out if exists.
Recording
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
Kill stale Wine processes before starting the X environment, then wait up to 5s for them to exit to prevent conflicts. Also blocks Back during prelaunch so cleanup can finish; fails fast if any Wine PIDs remain.
XServerScreenfinds running Wine PIDs, sends SIGKILL, and waits up to 5s before starting X; throws if any remain, with warnings logged.ProcessHelper.killProcessandProcessHelper.killAllWineProcesses(SIGKILL) to support the cleanup.BackHandlerinPluviaMainwhile the loading dialog is visible andSteamService.keepAliveis false to avoid interrupting prelaunch.Written for commit d9dbf7e. Summary will update on new commits.
Summary by CodeRabbit