refactor: pulseaudio server management and suspend behavior#1574
Merged
utkarshdalal merged 3 commits intoJun 12, 2026
Merged
Conversation
Remove the `pulseaudioSuspendBehavior` preference and its related process signal handling. The PulseAudio server is now consistently run as a daemon, and audio suspension/resumption is managed by dynamically loading and unloading the `module-aaudio-sink` via `pactl`. This simplifies the audio component's lifecycle and improves stability by leveraging standard PulseAudio control mechanisms.
Contributor
📝 WalkthroughWalkthroughRemoves the configurable pulseaudioSuspendBehavior setting across prefs, UI, container model, and refactors PulseAudioComponent to daemon-mode startup with pactl-based liveness checks, immediate pactl suspend/resume, and a simplified constructor using only socket config and low-latency flag. ChangesPulseAudio Suspend Behavior Removal and Lifecycle Refactor
Sequence DiagramsequenceDiagram
participant XServerScreen
participant PulseAudioComponent
participant ProcessHelper
participant System
XServerScreen->>PulseAudioComponent: new PulseAudioComponent(socketConfig, lowLatency)
PulseAudioComponent->>PulseAudioComponent: isServerRunning() (execPactlCommand -> ProcessHelper)
PulseAudioComponent->>ProcessHelper: execWithOutput("pactl info", ..., true)
ProcessHelper->>System: spawn pactl, capture output
System-->>ProcessHelper: pactl output
ProcessHelper-->>PulseAudioComponent: command output
PulseAudioComponent->>System: killall pulseaudio / start libpulseaudio.so via ProcessHelper
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/com/winlator/core/ProcessHelper.java`:
- Line 200: The current Log.d call in ProcessHelper that prints
Arrays.toString(splitCommand(command)) and Arrays.toString(envp) leaks secrets;
change the logging in the method that contains splitCommand(command) so it only
logs the executable name (derived from splitCommand(command)[0] or the command
string before the first space) and a redacted/whitelisted env map (e.g., include
only SAFE_KEYS like "PATH","HOME" and replace other keys/values with
"<REDACTED>"); do not print full argv or raw envp arrays and ensure any helper
used for formatting (e.g., splitCommand) is used only to extract the executable
name.
In
`@app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java`:
- Around line 163-172: The code currently treats isModuleLoaded as authoritative
but it’s only intent; change the logic so the flag is only flipped after
verifying the module/sink actually exists—do not set isModuleLoaded
unconditionally inside loadModule() and unloadModule(); instead, have
loadModule() verify success (e.g., confirm pactl returned a module id or that
isSinkAlive() becomes true) and then set isModuleLoaded = true, and have
unloadModule() confirm removal before setting isModuleLoaded = false; also
update the branch that uses isModuleLoaded (the block calling isSinkAlive(),
loadModule(), and updateSink(false)) to prefer a real check (call isSinkAlive()
or verify module presence) when deciding to attempt reload so failed pactl calls
don’t leave the flag true and prevent retries.
- Around line 96-98: The component wrongly assumes PulseAudio is up because
startPulseAudio() ignores subprocess failures and isServerRunning() treats any
output other than "connection refused" as healthy; update startPulseAudio() to
return a boolean (or throw) based on the actual process exit status and
stdout/stderr (or verify a successful pactl info), change isServerRunning() to
only report healthy when a positive success signal is observed (e.g., pactl info
returns expected fields or pid/socket exists), and then in start() (and the
other call sites that currently call killAllPulseAudioProcesses();
startPulseAudio();) use the boolean/exception to decide whether the server is
truly running and retry/abort the restart path when startPulseAudio() failed.
Ensure killAllPulseAudioProcesses(), startPulseAudio(), isServerRunning(), and
start() are updated accordingly so failures are propagated and logged.
🪄 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: 9434d940-c502-4091-8efa-ea80ebd84a79
📒 Files selected for processing (10)
app/src/main/assets/pulseaudio-gamenative-20260609.tzstapp/src/main/assets/pulseaudio-gamenative-20260612.tzstapp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/core/ProcessHelper.javaapp/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java
💤 Files with no reviewable changes (5)
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/java/com/winlator/container/ContainerData.kt
- app/src/main/java/com/winlator/container/Container.java
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java (1)
107-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't gate daemon recovery on
isPausedalone.If
pause()hits theisServerRunning()guard while the daemon is already down, it leavesisPaused=false.XEnvironment.onResume()still usesresume()as the audio recovery hook, but this method immediately returns in that state, so PulseAudio never gets restarted after the app comes back to foreground.Possible minimal fix
public void resume() { synchronized (lock) { + if (!isServerRunning()) { + start(); + return; + } + if (isPaused.get()) { Timber.tag("PulseAudioComponent").d("Resuming..."); - - if (isServerRunning()) { - // Set isPaused immediately - isPaused.set(false); - updateSink(false); - - Timber.tag("PulseAudioComponent").d("Audio resumed"); - } else { - start(); - } + isPaused.set(false); + updateSink(false); + Timber.tag("PulseAudioComponent").d("Audio resumed"); } } }Also applies to: 121-135
🤖 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/com/winlator/xenvironment/components/PulseAudioComponent.java` around lines 107 - 117, The pause() method should not leave isPaused false when the daemon is already down: set isPaused.set(true) unconditionally before checking isServerRunning(), then only call updateSink(true) and the "Pausing..." log if the server is running; keep the existing guarded behavior for updateSink/isServerRunning but ensure the paused flag is always true so XEnvironment.onResume()/resume() can perform recovery; apply the same unconditional isPaused set behavior to the analogous block in the 121-135 region.
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java (1)
20-38: ⚡ Quick winRefresh the class docblock.
It still describes timer-based suspend modes, SIGSTOP/SIGCONT handling, and module unload/reload behavior that no longer exists in this class. Leaving it in place makes the new lifecycle much harder to reason about.
🤖 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/com/winlator/xenvironment/components/PulseAudioComponent.java` around lines 20 - 38, The class docblock for PulseAudioComponent is outdated and describes timer-based suspend modes and SIGSTOP/SIGCONT/pactl behavior that no longer exist; replace it with a short, accurate summary of the current lifecycle and responsibilities of PulseAudioComponent, removing references to timers, SIGSTOP/SIGCONT, and module unload/reload. Mention the key state and API surfaces to guide readers (e.g., isPaused field, updateSink(...) method, and any public lifecycle methods such as start(), stop(), pause(), resume()) and note any important side-effects (threading assumptions or I/O interactions) so the docblock reflects the actual implementation and usage.
🤖 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.
Outside diff comments:
In
`@app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java`:
- Around line 107-117: The pause() method should not leave isPaused false when
the daemon is already down: set isPaused.set(true) unconditionally before
checking isServerRunning(), then only call updateSink(true) and the "Pausing..."
log if the server is running; keep the existing guarded behavior for
updateSink/isServerRunning but ensure the paused flag is always true so
XEnvironment.onResume()/resume() can perform recovery; apply the same
unconditional isPaused set behavior to the analogous block in the 121-135
region.
---
Nitpick comments:
In
`@app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java`:
- Around line 20-38: The class docblock for PulseAudioComponent is outdated and
describes timer-based suspend modes and SIGSTOP/SIGCONT/pactl behavior that no
longer exist; replace it with a short, accurate summary of the current lifecycle
and responsibilities of PulseAudioComponent, removing references to timers,
SIGSTOP/SIGCONT, and module unload/reload. Mention the key state and API
surfaces to guide readers (e.g., isPaused field, updateSink(...) method, and any
public lifecycle methods such as start(), stop(), pause(), resume()) and note
any important side-effects (threading assumptions or I/O interactions) so the
docblock reflects the actual implementation and usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 714122ac-d0fc-4e94-b0b0-1eed76ca8f22
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xenvironment/components/PulseAudioComponent.java
phobos665
approved these changes
Jun 12, 2026
phobos665
left a comment
phobos665
left a comment
Contributor
There was a problem hiding this comment.
Interesting. So we're removing a lot of complexity and instead leveraging the daemon instead. Nice.
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
3 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
Remove the
pulseaudioSuspendBehaviorpreference and its related process signal handling.The PulseAudio server is now consistently run as a daemon, and audio suspension/resumption is managed by dynamically suspending and resuming the
module-aaudio-sinkviapactl.This simplifies the audio component's lifecycle and improves stability by leveraging standard PulseAudio control mechanisms.
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
Run PulseAudio as a daemon and control pause/resume by toggling
pactl suspend-sinkonAAudioSink. Removes thepulseaudioSuspendBehaviorsetting and process-signal logic for a simpler, more stable audio flow.PulseAudioComponent: no process tracking or timers; server check viapactl info; pause/resume usespactl suspend-sink; low-latency stays.--daemonize=true,auth-cookie-enabled=false, clean~/.config, setPULSE_SERVER, load viadefault.pa; refreshed asset topulseaudio-gamenative-20260612.tzst.pulseaudioSuspendBehaviorfrom prefs, UI,Container, and serialization;XServerScreennow passes only low-latency.ProcessHelper: removed PID and duplicate exec helpers; consolidatedexecWithOutputwith better logging.Written for commit 0799d06. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Chores