fix: kill stale wine processes before launch#1195
Open
xXJSONDeruloXx wants to merge 4 commits intoutkarshdalal:masterfrom
Open
fix: kill stale wine processes before launch#1195xXJSONDeruloXx wants to merge 4 commits intoutkarshdalal:masterfrom
xXJSONDeruloXx wants to merge 4 commits intoutkarshdalal:masterfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds new hard-kill APIs to ProcessHelper (killProcess, killAllWineProcesses, hardKillStaleWineProcesses) and invokes hardKillStaleWineProcesses at the start of XServerScreen.setupXEnvironment(...). Also adds a BackHandler in PluviaMain to suppress Back 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->>Helper: hardKillStaleWineProcesses()
Helper->>OS: listRunningWineProcesses()
OS-->>Helper: current Wine PIDs
Helper->>Helper: log stale PIDs (if any)
Helper->>OS: killProcess(pid) for each PID (SIGKILL)
OS-->>Helper: kill acknowledgements / process state
Helper->>OS: poll listRunningWineProcesses() every 100ms (<=5s)
OS-->>Helper: remaining PIDs (empty or non-empty)
alt no remaining PIDs
Helper-->>XServer: completion
XServer->>XServer: continue setup
else remaining PIDs after timeout
Helper-->>XServer: throw IllegalStateException (remaining PIDs)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
6 tasks
Contributor
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
Contributor
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
| return environment | ||
| } | ||
|
|
||
| private fun hardKillStaleWineProcessesBeforeLaunch() { |
Owner
There was a problem hiding this comment.
remove from XServerScreen, it could be in processhelper or somewhere else. Generally we should not be adding to this file, it's already very bloated
| } | ||
| } | ||
|
|
||
| public static void killAllWineProcesses() { |
Owner
There was a problem hiding this comment.
we already have terminateAllWineProcesses is that not enough?
Contributor
Author
There was a problem hiding this comment.
thats the whole point, sigterm seems ignored in some cases, so sigkill is more aggressive which it needs to be on anything that remains in this state
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
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 and 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.
ProcessHelper.hardKillStaleWineProcesses();XServerScreencalls it before starting X. Sends SIGKILL to Wine PIDs, waits up to 5s, logs warnings, and throws if any remain.ProcessHelper.killProcess,ProcessHelper.killAllWineProcesses, andProcessHelper.hardKillStaleWineProcessesto support the cleanup.BackHandlerinPluviaMainwhile the loading dialog is visible andSteamService.keepAliveis false to avoid interrupting prelaunch.Written for commit ea948b5. Summary will update on new commits.
Summary by CodeRabbit