fix: clear stale container state on task swipe and app restart#1136
Conversation
|
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 (3)
📝 WalkthroughWalkthroughAdded a centralized teardown API Changes
Sequence Diagram(s)sequenceDiagram
participant MainActivity as MainActivity
participant SteamService as SteamService
participant PluviaApp as PluviaApp
participant XServerScreen as XServerScreen
MainActivity->>SteamService: read keepAlive
alt keepAlive && PluviaApp.xEnvironment == null
MainActivity->>PluviaApp: shutdownEnvironment()
PluviaApp->>PluviaApp: stop watchers, stop env components
PluviaApp->>SteamService: clearCachedAchievements()
PluviaApp->>SteamService: set keepAlive = false
PluviaApp->>PluviaApp: null refs, clear suspend state
end
XServerScreen->>XServerScreen: exit() -> winHandler?.stop()
XServerScreen->>PluviaApp: shutdownEnvironment()
MainActivity->>MainActivity: emit AndroidEvent.ActivityDestroyed (before super)
alt SteamService.keepAlive still true
MainActivity->>PluviaApp: shutdownEnvironment()
end
MainActivity->>MainActivity: super.onDestroy()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/MainActivity.kt`:
- Around line 279-287: The partial fallback in MainActivity (the block that
checks SteamService.keepAlive and calls env.stopEnvironmentComponents(), clears
PluviaApp.xEnvironment and SteamService.keepAlive, and calls
PluviaApp.clearActiveSuspendState()) must instead delegate to the same full
teardown routine used by the XServerScreen exit path; update the MainActivity
fallback to invoke the centralized cleanup function used by XServerScreen.exit()
(or refactor the XServerScreen exit logic into a shared
PluviaApp.performFullTeardown()/shutdownEnvironment() and call that) so
achievement watcher shutdown, cached-achievement reset, input/touchpad singleton
cleanup, and the existing stopEnvironmentComponents/keepAlive/xEnvironment
clearing all run in both paths.
- Around line 275-290: The ActivityDestroyed event is emitted unconditionally in
MainActivity.onDestroy which triggers terminal shutdown logic (exit()) during
configuration-change recreations; wrap the
PluviaApp.events.emit(AndroidEvent.ActivityDestroyed) call in a guard that
checks isChangingConfigurations (or isFinishing()) and only emit when the
activity is not being recreated (e.g., if (!isChangingConfigurations) {
PluviaApp.events.emit(AndroidEvent.ActivityDestroyed) }) so temporary rotations
don't trigger XServerScreen's listener and shutdown logic in exit().
🪄 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: b07f152e-4244-4ac2-a1f1-c794b095fb54
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/MainActivity.kt
jeremybernstein
force-pushed
the
jb/fix-task-swipe-container-stuck
branch
from
d66ab5a to
ecdd1a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/MainActivity.kt`:
- Around line 273-284: The new onDestroy path can race with the async
environment bootstrap in XServerScreen.kt that assigns PluviaApp.xEnvironment;
cancel or gate that setup work so it cannot assign or start the environment
after the Activity has been destroyed. Specifically, add cancellation or a
session/state check to the background bootstrap job in XServerScreen.kt (the
task that constructs/publishes PluviaApp.xEnvironment) so it either aborts if
onDestroy has run or isCancelled, or obtains a short-lived session token that
onDestroy invalidates before calling
PluviaApp.events.emit/PluviaApp.shutdownEnvironment; ensure the bootstrap checks
that token or !isDestroyed (or job.isActive) right before setting
PluviaApp.xEnvironment and starting services so a resurrected environment cannot
appear after onDestroy/shutdownEnvironment runs.
- Around line 147-150: The stale-session check currently uses
SteamService.keepAlive && PluviaApp.xEnvironment == null which also matches
non-XServer flows; change the guard to only clear the stale keepAlive when it
truly represents a game/XServer session by also checking the game-session
marker: call PluviaApp.hasValidSuspendPolicyState() and only invoke
PluviaApp.shutdownEnvironment() when SteamService.keepAlive is true,
PluviaApp.xEnvironment is null, and PluviaApp.hasValidSuspendPolicyState() is
false (i.e., gate the shutdown on the absence of a valid suspend policy state).
In `@app/src/main/java/app/gamenative/PluviaApp.kt`:
- Around line 204-225: shutdownEnvironment() must perform a complete,
exception-safe teardown: ensure the WinHandler.stop() cleanup and the null/flag
resets always run even if env?.stopEnvironmentComponents() throws. Modify
shutdownEnvironment() to call env?.stopEnvironmentComponents() inside a
try/catch (log the exception) and then in a finally block call
WinHandler.stop(), set xEnvironment = null, SteamService.keepAlive = false,
clearActiveSuspendState(), and null out
inputControlsView/inputControlsManager/touchpadView/achievementWatcher; also
move touchpadView?.releasePointerCapture() into the safe teardown flow. Ensure
XServerScreen.exit() uses this helper so both paths are identical.
🪄 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: 23707692-13cd-4d71-a1ab-a93472b79435
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
jeremybernstein
force-pushed
the
jb/fix-task-swipe-container-stuck
branch
from
ecdd1a9 to
809edb2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/PluviaApp.kt (1)
204-229:⚠️ Potential issue | 🟠 MajorFinish making
shutdownEnvironment()the full teardown path.This is much closer now, but
XServerScreen.exit()still has to stopWinHandlerseparately on Line 3395, so this helper is not actually the single source of truth yet. The singletryalso short-circuitsenv?.stopEnvironmentComponents()ifachievementWatcher?.stop(),SteamService.clearCachedAchievements(), ortouchpadView?.releasePointerCapture()throws first. SinceXEnvironment.stopEnvironmentComponents()does not stopWinHandler, callers that rely onshutdownEnvironment()alone can still leave the handler/socket alive or miss the actual environment stop.Suggested shape
+import com.winlator.xenvironment.components.XServerComponent + fun shutdownEnvironment() { val env = xEnvironment + val winHandler = env?.getComponent(XServerComponent::class.java)?.xServer?.winHandler Timber.i("shutdownEnvironment: env=%s", env != null) - try { - achievementWatcher?.stop() - achievementWatcher = null - SteamService.clearCachedAchievements() - touchpadView?.releasePointerCapture() - env?.stopEnvironmentComponents() - } catch (e: Exception) { - Timber.e(e, "shutdownEnvironment: error during component teardown") - } finally { - xEnvironment = null - inputControlsView = null - inputControlsManager = null - touchpadView = null - achievementWatcher = null - SteamService.keepAlive = false - clearActiveSuspendState() - } + runCatching { achievementWatcher?.stop() } + .onFailure { Timber.e(it, "shutdownEnvironment: error stopping achievement watcher") } + achievementWatcher = null + + runCatching { SteamService.clearCachedAchievements() } + .onFailure { Timber.e(it, "shutdownEnvironment: error clearing cached achievements") } + runCatching { touchpadView?.releasePointerCapture() } + .onFailure { Timber.e(it, "shutdownEnvironment: error releasing pointer capture") } + runCatching { winHandler?.stop() } + .onFailure { Timber.e(it, "shutdownEnvironment: error stopping WinHandler") } + runCatching { env?.stopEnvironmentComponents() } + .onFailure { Timber.e(it, "shutdownEnvironment: error during component teardown") } + + xEnvironment = null + inputControlsView = null + inputControlsManager = null + touchpadView = null + achievementWatcher = null + SteamService.keepAlive = false + clearActiveSuspendState() }After that,
XServerScreen.exit()can delegate completely instead of keeping a separate handler stop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/PluviaApp.kt` around lines 204 - 229, shutdownEnvironment currently short-circuits if early teardown steps throw and doesn't stop WinHandler/socket, so make it the single teardown path by ensuring all components run even if one fails and by stopping the WinHandler there; wrap each step (achievementWatcher?.stop(), SteamService.clearCachedAchievements(), touchpadView?.releasePointerCapture(), env?.stopEnvironmentComponents(), and an explicit WinHandler.stop()/socket close) in their own try/catch or otherwise catch exceptions per-step so later cleanup still runs, call the WinHandler stop/socket close from shutdownEnvironment (or have XEnvironment.stopEnvironmentComponents include WinHandler shutdown) and then remove the separate WinHandler stop from XServerScreen.exit so callers can delegate solely to shutdownEnvironment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/PluviaApp.kt`:
- Around line 204-229: shutdownEnvironment currently short-circuits if early
teardown steps throw and doesn't stop WinHandler/socket, so make it the single
teardown path by ensuring all components run even if one fails and by stopping
the WinHandler there; wrap each step (achievementWatcher?.stop(),
SteamService.clearCachedAchievements(), touchpadView?.releasePointerCapture(),
env?.stopEnvironmentComponents(), and an explicit WinHandler.stop()/socket
close) in their own try/catch or otherwise catch exceptions per-step so later
cleanup still runs, call the WinHandler stop/socket close from
shutdownEnvironment (or have XEnvironment.stopEnvironmentComponents include
WinHandler shutdown) and then remove the separate WinHandler stop from
XServerScreen.exit so callers can delegate solely to shutdownEnvironment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44199d61-7e79-4184-a845-82f9d8b849bc
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
@coderabbitai: I disagree that we need to stop WinHandler in the case of a swipe. The OS will reclaim the socket on its own. It's important for the exit() case, but seems like unjustified extra complexity here. The single try/catch finding makes sense to me, will be momentarily addressed in a force-push. |
jeremybernstein
force-pushed
the
jb/fix-task-swipe-container-stuck
branch
from
809edb2 to
760adaf
Compare
|
Tip For best results, initiate chat on the files or code changes.
Thanks for confirming on the ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
3360-3407: LGTM! Clean centralization of teardown logic.The refactoring correctly delegates cleanup to
PluviaApp.shutdownEnvironment()while preserving the explicitwinHandler?.stop()call for graceful exits. The comment at line 3395 clearly documents the design rationale.One minor observation: the
environment: XEnvironment?parameter (line 3362) is now unused sinceshutdownEnvironment()accessesPluviaApp.xEnvironmentdirectly. Consider removing it to clean up the API surface, though this is optional since maintaining the current signature avoids changing all 5 call sites.Based on learnings:
shutdownEnvironment()intentionally does not callwinHandler?.stop()— on task-swipe the OS reclaims the socket naturally, and the explicit stop beforeshutdownEnvironment()covers the graceful-exit case.🤖 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/xserver/XServerScreen.kt` around lines 3360 - 3407, The parameter environment: XEnvironment? on the exit(...) function is now unused because shutdown is done via PluviaApp.shutdownEnvironment(); remove the environment parameter from the exit function signature (function name: exit) and update all call sites (there are 5) to stop passing that argument, also remove any now-unused imports or references to environment inside the file; ensure method references or lambda types that depended on the old signature are updated accordingly so compilation succeeds.
🤖 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/xserver/XServerScreen.kt`:
- Around line 3360-3407: The parameter environment: XEnvironment? on the
exit(...) function is now unused because shutdown is done via
PluviaApp.shutdownEnvironment(); remove the environment parameter from the exit
function signature (function name: exit) and update all call sites (there are 5)
to stop passing that argument, also remove any now-unused imports or references
to environment inside the file; ensure method references or lambda types that
depended on the old signature are updated accordingly so compilation succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54491248-12b8-4f1d-8276-a9802639e54e
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/MainActivity.kt
- app/src/main/java/app/gamenative/PluviaApp.kt
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/screen/xserver/XServerScreen.kt`:
- Around line 3393-3395: The unguarded call to winHandler?.stop() in the exit
path can throw and prevent PluviaApp.shutdownEnvironment() from running; wrap
the winHandler stop call in a try/catch inside the same exit/teardown routine
(catch Throwable/Exception) and ensure the catch only logs the error (including
the exception) so that PluviaApp.shutdownEnvironment() is always invoked;
reference the winHandler?.stop() invocation and the
PluviaApp.shutdownEnvironment() call to locate where to add the try/catch and
logging.
🪄 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: b89e9634-8787-489b-9a54-48e62e2254f6
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
jeremybernstein
force-pushed
the
jb/fix-task-swipe-container-stuck
branch
from
b247853 to
61b8a9d
Compare
|
I've confirmed this working on my phone. |
|
Can we minimize the scope of this? Looks like the only necessary changes are in MainActivity, can we just do that and leave the rest alone? |
|
also there are conflicts |
☝️ exactly, because we're shutting down. In theory, those resources will be reclaimed by the OS on process death, but if the process survives (e.g. service keeps it alive) they leak/bleed into the next container launch. In fact, that's what happens on my phone: I swipe the game away and the audio keeps playing (and other services remain) until I launch the container again, at which point a) the audio/services stop, and the b) the container doesn't launch. 😄 I think the complete shutdown is the correct solution here. Fixing the conflicts. |
when the user swipes the app from the task switcher while a container is running, keepAlive stays true but xEnvironment is gone. on next launch the app is stuck thinking a container is running. extract shutdownEnvironment() from XServerScreen.exit() so the same full teardown runs in both the normal exit path and the crash recovery path. each step is wrapped in runCatching so one failure doesn't prevent the rest. - onCreate: if keepAlive is set but xEnvironment is null, run shutdownEnvironment() to clear stale state - onDestroy: emit ActivityDestroyed before super (so exit() listeners still fire), then force shutdownEnvironment() if keepAlive persists - exit(): delegates teardown to shutdownEnvironment(), keeps only winHandler.stop() and trash cleanup (container-specific)
jeremybernstein
force-pushed
the
jb/fix-task-swipe-container-stuck
branch
from
61b8a9d to
c9ae6c2
Compare
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
I just noticed that the GN notification doesn't go away when you swipe the app, but now it does (2nd commit just pushed). |
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/service/gog/GOGService.kt`:
- Around line 698-706: onTaskRemoved currently calls stopSelf() based solely on
hasActiveOperations(), which doesn't account for active downloads; update the
onTaskRemoved method to check for active downloads before stopping the service
(combine hasActiveOperations() with the service's download state, e.g. a
method/field like isDownloading(), downloadManager.hasActiveDownloads(), or
activeDownloads count in GOGService) and only call stopSelf() when neither
operations nor downloads are active; keep the existing Timber logs and adjust
the condition to use the combined check.
🪄 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: bdabad7f-3517-4e49-abcc-a12949aef093
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/epic/EpicService.ktapp/src/main/java/app/gamenative/service/gog/GOGService.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/service/gog/GOGService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGService.kt:700">
P1: `onTaskRemoved` may stop the service during active downloads because it checks only sync state via `hasActiveOperations()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Steam/GOG/Epic services had no onTaskRemoved — foreground notification persisted after swipe because nothing told the service to stop itself.
jeremybernstein
force-pushed
the
jb/fix-task-swipe-container-stuck
branch
from
c887b7a to
1989857
Compare
|
arguably the notification stuff should be its own PR, but it's obviously closely related, so avoiding the overhead. |
|
@utkarshdalal @jeremybernstein if worried about touching sensitive portions of the code, do you think this would be a less invasive bandaid that solves the end user hang for now, then we can solve for the core teardown issues more methodically? Currently I think this is our biggest bug and I would even say a hot fix release might be warranted given how often its coming up #1195 |
Thanks @xXJSONDeruloXx ! I tried a variety of combinations to see what helped. If we don't teardown on swipe, the wine process continues to run (so f.e. I have a game with a title music loop, if I swipe it away, the title music continues to play and won't stop without a Force Quit). I appreciate the desire for minimal fixes, and that's always my goal and design priority when tackling issues in this codebase. Sadly, sometimes you need to change code to fix code. The main change here is a refactor of existing shutdown code into a reusable function so it can be called from elsewhere, and then calling it from elsewhere, when shutting down the software -- that doesn't seem controversial to me. As a bonus, it solves the problem. The new second commit ensures that the notification badge disappears. It can be eliminated if the priority is minimal LoC, but then you'll have a notification badge hanging around. |
|
i've merged but we'll need to kill AmazonService too, no? |
…shdalal#1136) * fix: clear stale container state on task swipe and app restart when the user swipes the app from the task switcher while a container is running, keepAlive stays true but xEnvironment is gone. on next launch the app is stuck thinking a container is running. extract shutdownEnvironment() from XServerScreen.exit() so the same full teardown runs in both the normal exit path and the crash recovery path. each step is wrapped in runCatching so one failure doesn't prevent the rest. - onCreate: if keepAlive is set but xEnvironment is null, run shutdownEnvironment() to clear stale state - onDestroy: emit ActivityDestroyed before super (so exit() listeners still fire), then force shutdownEnvironment() if keepAlive persists - exit(): delegates teardown to shutdownEnvironment(), keeps only winHandler.stop() and trash cleanup (container-specific) * fix: stop all foreground services on task swipe when idle Steam/GOG/Epic services had no onTaskRemoved — foreground notification persisted after swipe because nothing told the service to stop itself.
Amazon already had |
3 participants
Description
Fixes app getting stuck on the container screen after task-swiping during a game.
onDestroywas callingsuper.onDestroy()before emittingActivityDestroyed, so ComposeDisposableEffectlisteners (includingexit()) were already unregistered and never fired. Even when re-ordered, a race between listener teardown and event emission can still leave stale state.onDestroysoActivityDestroyedfires before lifecycle teardown, givingexit()a chance to runonDestroywhenexit()doesn't fire (listener race / task swipe)keepAliveflag inonCreatewhen no container is actually running (crash recovery)Closes #1135
Recording
n/a — no visible UI changes
Test plan
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by CodeRabbit
Bug Fixes
Refactor