From db6955f5b7bf036dc010efcb1f722071039dfb76 Mon Sep 17 00:00:00 2001 From: deedra Date: Wed, 28 Oct 2026 08:57:12 -0700 Subject: [PATCH] Improve sound server fallback and speech acknowledgement handling Try the invoking user session followed by /run/pulse/native for sound cues, while preserving an explicit PULSE_SERVER override. Resolve the user socket from SUDO_UID, XDG_RUNTIME_DIR, or the current UID, and document the fallback and Speech Dispatcher audio configuration. Allow five seconds for Speech Dispatcher command acknowledgements so large voice catalog queries can finish without premature failures. Add regression coverage for socket selection and delayed replies. Include cpu-usage.patch as a standalone patch artifact containing Python remote registry permission and registration failure handling changes. The patch is stored without applying it to the Python sources. Validation: go test ./internal/sound ./internal/speech -count=1 passed with GOCACHE=/tmp/cthulhu-go-cache and GOPROXY=off outside the sandbox to permit local Unix sockets. source diff whitespace checks passed. The staged patch has whitespace warnings from preserved blank context lines; its contents are retained unchanged. Live audio and keyboard behavior were not tested. --- README.md | 5 ++ cpu-usage.patch | 123 +++++++++++++++++++++++++++++++++ internal/sound/pulse.go | 8 +-- internal/sound/service.go | 12 +++- internal/sound/service_test.go | 15 +++- internal/speech/client.go | 5 +- internal/speech/client_test.go | 19 +++++ 7 files changed, 177 insertions(+), 10 deletions(-) create mode 100644 cpu-usage.patch diff --git a/README.md b/README.md index 2c77068b..69310ff8 100644 --- a/README.md +++ b/README.md @@ -74,6 +74,11 @@ server or ALSA device. The [PulseAudio](tools/configure_pulse.sh) and [PipeWire](tools/configure_pipewire.sh) configuration scripts are included for systems that need root audio access. +The default sound driver tries the user session first, then the system-wide +PulseAudio-compatible socket at `/run/pulse/native`, including when provided by +PipeWire. Set `PULSE_SERVER` to select a different socket or server explicitly. +Speech output uses Speech Dispatcher's own audio configuration. + ### X11 terminal From an X11 terminal that supplies `WINDOWID`: diff --git a/cpu-usage.patch b/cpu-usage.patch new file mode 100644 index 00000000..f77e6cd6 --- /dev/null +++ b/cpu-usage.patch @@ -0,0 +1,123 @@ +diff --git a/src/fenrirscreenreader/core/remoteInstanceRegistry.py b/src/fenrirscreenreader/core/remoteInstanceRegistry.py +index 5733f3ab..07d707af 100644 +--- a/src/fenrirscreenreader/core/remoteInstanceRegistry.py ++++ b/src/fenrirscreenreader/core/remoteInstanceRegistry.py +@@ -12,6 +12,7 @@ import time + + + INSTANCE_TIMEOUT_SEC = 30.0 ++REGISTRY_DIR_MODE = 0o1777 + + + def get_registry_dir(): +@@ -26,8 +27,8 @@ def get_instance_file(pid=None): + + def write_instance(instance_data): + registry_dir = get_registry_dir() +- os.makedirs(registry_dir, mode=0o755, exist_ok=True) +- os.chmod(registry_dir, 0o755) ++ os.makedirs(registry_dir, mode=REGISTRY_DIR_MODE, exist_ok=True) ++ os.chmod(registry_dir, REGISTRY_DIR_MODE) + prune_stale_instances() + instance_data["updated_at"] = time.time() + instance_path = get_instance_file(instance_data.get("pid")) +diff --git a/src/fenrirscreenreader/remoteDriver/unixDriver.py b/src/fenrirscreenreader/remoteDriver/unixDriver.py +index e7411184..720671cb 100644 +--- a/src/fenrirscreenreader/remoteDriver/unixDriver.py ++++ b/src/fenrirscreenreader/remoteDriver/unixDriver.py +@@ -114,6 +114,17 @@ class driver(remoteDriver): + } + remoteInstanceRegistry.write_instance(instance_data) + ++ def _try_register_instance(self): ++ try: ++ self._register_instance() ++ return True ++ except Exception as e: ++ self.env["runtime"]["DebugManager"].write_debug_out( ++ "unixDriver could not register instance: " + str(e), ++ debug.DebugLevel.ERROR, ++ ) ++ return False ++ + def _socket_file_matches_socket(self, fenrir_sock, socket_file): + try: + if os.stat(socket_file).st_uid != os.getuid(): +@@ -328,11 +339,11 @@ class driver(remoteDriver): + if not self.fenrirSocks: + return + +- self._register_instance() ++ self._try_register_instance() + last_register = time.time() + while active.value: + if time.time() - last_register > 10.0: +- self._register_instance() ++ self._try_register_instance() + last_register = time.time() + + # Check if the client is still connected and if data is available: +diff --git a/tests/unit/test_remote_instance_registry.py b/tests/unit/test_remote_instance_registry.py +index 7955665a..1b9aec28 100644 +--- a/tests/unit/test_remote_instance_registry.py ++++ b/tests/unit/test_remote_instance_registry.py +@@ -1,9 +1,24 @@ + import json ++import stat + import time + + from fenrirscreenreader.core import remoteInstanceRegistry + + ++def test_write_instance_creates_shared_sticky_registry_directory( ++ tmp_path, monkeypatch ++): ++ registry_dir = tmp_path / "instances" ++ monkeypatch.setattr( ++ remoteInstanceRegistry, "get_registry_dir", lambda: str(registry_dir) ++ ) ++ ++ remoteInstanceRegistry.write_instance({"pid": 456}) ++ ++ mode = stat.S_IMODE(registry_dir.stat().st_mode) ++ assert mode == 0o1777 ++ ++ + def test_write_instance_prunes_stale_registry_files(tmp_path, monkeypatch): + monkeypatch.setattr( + remoteInstanceRegistry, "get_registry_dir", lambda: str(tmp_path) +diff --git a/tests/unit/test_unix_remote_driver.py b/tests/unit/test_unix_remote_driver.py +index ea95d816..325aa733 100644 +--- a/tests/unit/test_unix_remote_driver.py ++++ b/tests/unit/test_unix_remote_driver.py +@@ -3,6 +3,30 @@ from unittest.mock import Mock, mock_open, patch + from fenrirscreenreader.remoteDriver import unixDriver + + ++def test_watchdog_keeps_serving_when_instance_registration_fails( ++ mock_environment, ++): ++ driver = unixDriver.driver() ++ driver.env = mock_environment ++ driver.fenrirSocks = [Mock()] ++ active = Mock(value=True) ++ event_queue = Mock() ++ ++ def stop_after_select(*_args): ++ active.value = False ++ return ([], [], []) ++ ++ with patch.object(driver, "_get_socket_candidates", return_value=[]), patch.object( ++ driver, "_register_instance", side_effect=PermissionError("denied") ++ ), patch( ++ "fenrirscreenreader.remoteDriver.unixDriver.select.select", ++ side_effect=stop_after_select, ++ ) as select_call: ++ driver.watch_dog(active, event_queue) ++ ++ select_call.assert_called_once() ++ ++ + class FakeClientSocket: + def __init__(self, data): + self.data = data diff --git a/internal/sound/pulse.go b/internal/sound/pulse.go index e7b5b2de..1f996202 100644 --- a/internal/sound/pulse.go +++ b/internal/sound/pulse.go @@ -29,11 +29,9 @@ import ( func open_pulse(s *Service, _ string) error { options := []pulse.ClientOption{pulse.ClientApplicationName("Fenrir sounds"), pulse.ClientTimeout(500 * time.Millisecond)} - // Match the invoking user's session when sudo strips XDG_RUNTIME_DIR. - // An explicit PULSE_SERVER continues to take precedence. - if server := Server(os.Getenv("PULSE_SERVER"), os.Getenv("SUDO_UID")); server != "" { - options = append(options, pulse.ClientServerString(server)) - } + // Try the invoking user's session, then the system-wide service, unless + // PULSE_SERVER explicitly selects a server. + options = append(options, pulse.ClientServerString(Server(os.Getenv("PULSE_SERVER"), os.Getenv("SUDO_UID")))) client, err := pulse.NewClient(options...) if err != nil { return fmt.Errorf("connect sound server: %w", err) diff --git a/internal/sound/service.go b/internal/sound/service.go index 1a7ae9c5..5161653f 100644 --- a/internal/sound/service.go +++ b/internal/sound/service.go @@ -23,6 +23,8 @@ import ( "errors" "fmt" "math" + "os" + "path/filepath" "strconv" "sync" "sync/atomic" @@ -84,14 +86,20 @@ func (s *Service) Set_volume(volume float64) { s.volume.Store(math.Float32bits(float32(volume))) } +// Server returns PulseAudio protocol addresses in connection order. PipeWire's +// PulseAudio service can expose the system-wide /run/pulse/native socket. +// The client library tries each address, including after authentication errors. func Server(explicit, sudoUID string) string { if explicit != "" { return explicit } + runtimeDir := os.Getenv("XDG_RUNTIME_DIR") if uid, err := strconv.ParseUint(sudoUID, 10, 32); err == nil { - return fmt.Sprintf("unix:/run/user/%d/pulse/native", uid) + runtimeDir = fmt.Sprintf("/run/user/%d", uid) + } else if runtimeDir == "" { + runtimeDir = fmt.Sprintf("/run/user/%d", os.Getuid()) } - return "" + return "unix:" + filepath.Join(runtimeDir, "pulse/native") + " unix:/run/pulse/native" } // Play only replaces a mailbox pointer. Startup has its own slot so initial diff --git a/internal/sound/service_test.go b/internal/sound/service_test.go index aaece80a..3f22880a 100644 --- a/internal/sound/service_test.go +++ b/internal/sound/service_test.go @@ -20,6 +20,8 @@ package sound import ( "context" "errors" + "fmt" + "os" "runtime" "slices" "sync" @@ -264,19 +266,28 @@ func TestPlayReportsUnavailableCues(t *testing.T) { } func TestSoundServerFollowsExplicitAndSudoSession(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", "/run/user/2000") if got := Server("unix:/tmp/example-pulse", "1000"); got != "unix:/tmp/example-pulse" { t.Fatal(got) } - if got := Server("", "1000"); got != "unix:/run/user/1000/pulse/native" { + if got := Server("", "1000"); got != "unix:/run/user/1000/pulse/native unix:/run/pulse/native" { t.Fatal(got) } for _, uid := range []string{"", "invalid", "../other"} { - if got := Server("", uid); got != "" { + if got := Server("", uid); got != "unix:/run/user/2000/pulse/native unix:/run/pulse/native" { t.Fatal(got) } } } +func TestSoundServerWithoutRuntimeDirectoryUsesCurrentUserAndSystemFallback(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", "") + want := fmt.Sprintf("unix:/run/user/%d/pulse/native unix:/run/pulse/native", os.Getuid()) + if got := Server("", ""); got != want { + t.Fatalf("Server() = %q, want %q", got, want) + } +} + func BenchmarkCueAdmission(b *testing.B) { s := &Service{clips: map[presentation.Cue]*clip{presentation.Blank: {samples: []float32{1, 1}}}} s.Set_volume(1) diff --git a/internal/speech/client.go b/internal/speech/client.go index 358a649b..2e8e7f4b 100644 --- a/internal/speech/client.go +++ b/internal/speech/client.go @@ -34,7 +34,10 @@ import ( "git.stormux.org/storm/fenrir/internal/diagnostic" ) -const commandTimeout = 750 * time.Millisecond +// Speech Dispatcher serializes some module queries with speech commands. +// Large voice catalogs can delay acknowledgements beyond a fraction of a second. +// Keep the wait bounded while allowing those queries to finish. +const commandTimeout = 5 * time.Second type Rejection struct{ Code string } diff --git a/internal/speech/client_test.go b/internal/speech/client_test.go index 74ae2e62..3abf84ec 100644 --- a/internal/speech/client_test.go +++ b/internal/speech/client_test.go @@ -25,8 +25,27 @@ import ( "strings" "sync" "testing" + "time" ) +func TestSpeechAcknowledgementWaitsForBusyDispatcher(t *testing.T) { + server := new_fake_server(t, true) + client, err := Open(Options{Socket: server.path}) + if err != nil { + t.Fatal(err) + } + defer client.Close() + result := make(chan error, 1) + go func() { result <- client.Say("startup") }() + wait_event(t, server.events, "say:startup") + // Voice catalog requests can occupy the dispatcher for more than 750 ms. + time.Sleep(time.Second) + close(server.ack) + if err := <-result; err != nil { + t.Fatalf("speech failed while dispatcher was busy: %v", err) + } +} + type fakeServer struct { path string events chan string