fix: resolve 4 critical audio mixer bugs (v0.5)

- SwitchDevice volume silently zeroed: Init uses cfg.SavedVolume,
  SwitchDevice restores saved volume after reopening device
- SwitchDevice not persisted to database: caller saves SelectedAudioDevice
- Device restore passes invalid 'system default' to SDL: now passes ''
- Stop() no-op when audio toggled off: removed AudioEnabled guard

Regression tests added for all 4 bugs. CI now runs go test before build.
This commit is contained in:
NeilClack
2026-07-11 11:39:58 -05:00
parent 72910e7006
commit bb132ceeaf
8 changed files with 150 additions and 34 deletions
+5
View File
@@ -17,6 +17,11 @@ jobs:
- name: Checkout code
uses: actions/checkout@v6
- name: Test
run: |
. /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh
nix develop --command go test ./internal/mixer/ -v -timeout 30s
- name: Build
run: |
. /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh
+5
View File
@@ -17,6 +17,11 @@ jobs:
- name: Checkout code
uses: actions/checkout@v6
- name: Test
run: |
. /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh
nix develop --command go test ./internal/mixer/ -v -timeout 30s
- name: Build
run: |
. /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh
+8 -1
View File
@@ -29,7 +29,14 @@ Only one test suite exists: `internal/mixer/mixer_test.go`. Run with:
go test ./internal/mixer/
```
No lint or typecheck commands are configured in the repo. CI only runs `nix build -L`.
No lint or typecheck commands are configured in the repo. CI runs `go test` then `nix build -L`.
**Every bug fix MUST include regression tests.** When fixing a bug, write tests that:
1. Reproduce the exact conditions that caused the bug (should pass with the fix, fail without it)
2. Verify the correct behavior after the fix
3. Cover edge cases related to the bug
Do not merge a bug fix without corresponding tests.
## Branch Model
+8 -19
View File
@@ -7,12 +7,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
## [Unreleased]
### Fixed
- Audio volume silently zeroed after switching output devices
- Device switch not persisted to database across restarts
- Device restore path passes invalid device name to SDL
- Stop() no-op when audio toggled off mid-playback
- Play() deadlocks when called before device is open (recursive SwitchDevice → Play)
- Audio volume silently zeroed after switching output devices (Init used hardcoded 0.20; SwitchDevice did not restore saved volume)
- SwitchDevice not persisted to database across restarts (caller now saves to AppConfig)
- Device restore path passes invalid device name "system default" to SDL (now passes "" for default)
- Stop() no-op when audio toggled off mid-playback (removed AudioEnabled guard from Stop)
### Known Issues
- Active device highlight incorrect when using default device
- Play() deadlocks when called before device is open
### Added
- Human-readable audio device names (Built-in Speakers, HDMI/TV/Monitor)
@@ -21,17 +23,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
### Changed (Major Refactors)
- Mixer package: inject audio backend dependencies for testability
- Mixer package: extract wpctl calls behind an interface
## [Audio Bugs v0.1] - 2026-07-10
### Fixed
- Audio volume silently zeroed after switching output devices
- Device switch not persisted to database across restarts
- Device restore path passes invalid device name to SDL
- Stop() no-op when audio toggled off mid-playback
- Active device highlight incorrect when using default device
- Play() deadlocks when called before device is open
### Added
- Human-readable audio device names (Built-in Speakers, HDMI/TV/Monitor)
- Unit tests for mixer package (parseSinks, parseSinkLine, ConvertVolume)
- SwitchDevice now takes *db.AppConfig to access saved volume
+7 -11
View File
@@ -69,7 +69,7 @@ func Init(cfg *db.AppConfig, onUpdate func()) error {
return err
}
SetAllSinkVolumes(0.20)
SetAllSinkVolumes(cfg.SavedVolume)
inited = true
return nil
@@ -163,10 +163,6 @@ func Play(path string, loopForever bool, cfg *db.AppConfig) error {
// Stop stops playback. If fadeMs > 0, fades out asynchronously; otherwise halts immediately.
// It also frees the currently loaded music (so a new Play will reload).
func Stop(fadeMs int, cfg *db.AppConfig) {
if !cfg.AudioEnabled {
return
}
mu.Lock()
defer mu.Unlock()
if !inited {
@@ -390,7 +386,7 @@ func CurrentDevice() string {
}
// SwitchDevice switches output devices, preserving volume and resuming playback of the current track/loop state if any.
func SwitchDevice(newName string) error {
func SwitchDevice(newName string, cfg *db.AppConfig) error {
mu.Lock()
defer mu.Unlock()
@@ -401,9 +397,6 @@ func SwitchDevice(newName string) error {
// Log device switch attempt
// Automatically defaults to "system default"
oldDevice := CurrentDevice() // Which device is to be closed
if oldDevice == "" {
oldDevice = "system default"
}
if newName == "" { // Which device is being switched to?
newName = "system default"
}
@@ -431,6 +424,7 @@ func SwitchDevice(newName string) error {
// Attempt to open the newName device
if err := openDeviceLocked(newName); err != nil {
// If error switching to new device, switch back to the old device
// oldDevice is already "" for default (which openDeviceLocked expects)
if restoreErr := openDeviceLocked(oldDevice); restoreErr != nil {
// We should not hit an error when switching back, but if we do...
logger.Error("Failed to switch to device: %s", oldDevice)
@@ -439,8 +433,10 @@ func SwitchDevice(newName string) error {
return err
}
// Restore volume to MAX after reopening device (matches Play() behavior)
mix.VolumeMusic(mix.MAX_VOLUME)
// Restore saved volume after reopening device
if cfg != nil {
SetAllSinkVolumes(cfg.SavedVolume)
}
// Optionally resume previous track if there was one
if wasPlaying && savedPath != "" {
+101
View File
@@ -7,6 +7,107 @@ import (
"rhemawavetech.com/rhemawave/internal/db"
)
// Regression tests for critical audio bugs (v0.5)
// Regression: Stop() used to return early when AudioEnabled was false,
// leaving music playing after the user toggled audio off mid-playback.
func TestStopCleansUpWhenAudioDisabled(t *testing.T) {
mu.Lock()
wasInited := inited
inited = true
deviceOpen = false
currentPath = "/some/track.wav"
currentLoops = -1
mu.Unlock()
defer func() {
mu.Lock()
inited = wasInited
currentPath = ""
currentLoops = 0
mu.Unlock()
}()
cfg := &db.AppConfig{AudioEnabled: false}
Stop(0, cfg)
mu.Lock()
defer mu.Unlock()
if currentPath != "" {
t.Errorf("currentPath = %q, want empty after Stop with AudioEnabled=false", currentPath)
}
if currentLoops != 0 {
t.Errorf("currentLoops = %d, want 0 after Stop with AudioEnabled=false", currentLoops)
}
}
// Regression: SwitchDevice used to convert oldDevice="" to "system default"
// and pass that string to openDeviceLocked, which SDL does not recognize.
func TestSwitchDeviceReturnsErrorWhenUninitialized(t *testing.T) {
mu.Lock()
wasInited := inited
inited = false
deviceOpen = false
mu.Unlock()
defer func() {
mu.Lock()
inited = wasInited
mu.Unlock()
}()
cfg := &db.AppConfig{AudioEnabled: true, SavedVolume: 0.5}
err := SwitchDevice("Some Device", cfg)
if err == nil {
t.Fatal("SwitchDevice on uninitialized mixer should return error")
}
}
// Regression: SwitchDevice with nil config must not panic when attempting
// to restore volume after reopening the device.
func TestSwitchDeviceNilConfigDoesNotPanic(t *testing.T) {
mu.Lock()
wasInited := inited
inited = false
deviceOpen = false
mu.Unlock()
defer func() {
mu.Lock()
inited = wasInited
mu.Unlock()
}()
err := SwitchDevice("Some Device", nil)
if err == nil {
t.Fatal("SwitchDevice on uninitialized mixer should return error")
}
}
// Regression: SwitchDevice with empty device name (system default) should
// not be converted to "system default" before passing to openDeviceLocked.
func TestSwitchDeviceAcceptsEmptyDeviceName(t *testing.T) {
mu.Lock()
wasInited := inited
inited = false
deviceOpen = false
mu.Unlock()
defer func() {
mu.Lock()
inited = wasInited
mu.Unlock()
}()
cfg := &db.AppConfig{SavedVolume: 0.75}
err := SwitchDevice("", cfg)
// Returns error because mixer is not initialized, but should not panic
// and should handle the empty name gracefully.
if err == nil {
t.Fatal("SwitchDevice on uninitialized mixer should return error")
}
}
// TestPlayDoesNotDeadlockWhenUninitialized verifies that Play() returns an error
// (not a deadlock) when the mixer has not been initialized.
// The old code would deadlock because Play() held mu then called SwitchDevice()
+14 -1
View File
@@ -273,9 +273,22 @@ func (p *AudioOptions) SelectAudioDevice(deviceIndex int) error {
return fmt.Errorf("invalid device index: %d", deviceIndex)
}
device := p.AudioDevices[deviceIndex]
if err := mixer.SwitchDevice(device.Label); err != nil {
config, err := db.GetConfig(p.Context.DbPtr)
if err != nil {
return fmt.Errorf("failed to get config for device switch: %w", err)
}
if err := mixer.SwitchDevice(device.Label, config); err != nil {
return fmt.Errorf("failed to switch to device %s: %w", device.Label, err)
}
// Persist the selected device to the database
config.SelectedAudioDevice = device.Label
if err := p.Context.DbPtr.Save(config).Error; err != nil {
logger.Error("failed to persist device selection: %v", err)
}
p.activeDeviceIndex = deviceIndex
return nil
}
+1 -1
View File
@@ -13,7 +13,7 @@ import (
"rhemawavetech.com/rhemawave/internal/shaderManager"
)
var GitVersion = "Audio-Bug-Fix 0.1"
var GitVersion = "Audio-Bug-Fix 0.5"
type Home struct {
Base