From 16d522b92f3bc310d3ad62fcb42613d24db3b61f Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Mon, 5 Oct 2026 01:48:54 +1300 Subject: [PATCH 01/15] fix(serve): sample play_start_us from CLOCK_MONOTONIC_RAW (#282) ## Problem In `sendspin/serve/coordinator.py`, `play_start_us` is sampled with `time.monotonic()` -- but the aiosendspin server clock is `CLOCK_MONOTONIC_RAW` on Linux (`aiosendspin/clock.py`: not slewed by NTP/adjtime). `play_start_us` is an absolute timestamp consumed verbatim by workers/clients, so sampling it from a different clock domain introduces a fixed offset equal to every NTP adjustment since boot. ## Evidence On my Linux host the two clocks are currently 102.2 s apart, so the multi-worker serve path timestamps audio ~102 s off. One honest caveat: `aiosendspin/clock.py` notes the rate mismatch is capped at ~500 ppm and below backpressure tolerances -- but that covers relative durations, not an absolute epoch offset, which is what `play_start_us` is. ## Fix Adds a small `server_now_us()` helper (raw clock on Linux, `monotonic_ns` fallback elsewhere) and uses it for the two absolute-timestamp samples. Interval-only measurements (stats/health timers) deliberately stay on `time.monotonic()`. ## Verification - `pytest tests/serve/test_coordinator.py`: 7 passed; `ruff check` clean - Live check: helper returns the raw-domain value Happy to adjust the approach if you prefer reusing an aiosendspin clock helper. --- sendspin/serve/coordinator.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/sendspin/serve/coordinator.py b/sendspin/serve/coordinator.py index 164c9f7..f8ca5ca 100644 --- a/sendspin/serve/coordinator.py +++ b/sendspin/serve/coordinator.py @@ -35,6 +35,24 @@ MAX_BUFFER_AHEAD_US = 5_000_000 +def server_now_us() -> int: + """Current time in microseconds on the aiosendspin server clock domain. + + The aiosendspin server timestamps audio with ``RawMonotonicClock`` + (``CLOCK_MONOTONIC_RAW`` on Linux), which — unlike + ``time.monotonic()`` — is not slewed by NTP/adjtime + (``aiosendspin/clock.py``). ``play_start_us`` is an absolute timestamp + consumed verbatim by workers/clients, so it must be sampled from the + same clock domain; ``time.monotonic()`` drifts away from it by every + NTP adjustment since boot (measured 102 s apart on one Linux host). + Interval-only measurements (stats/health timers) stay on + ``time.monotonic()``, which is the correct clock for durations. + """ + if hasattr(time, "CLOCK_MONOTONIC_RAW"): + return time.clock_gettime_ns(time.CLOCK_MONOTONIC_RAW) // 1_000 + return time.monotonic_ns() // 1_000 + + class ServeCoordinator: """Orchestrates multi-worker serve mode.""" @@ -243,7 +261,7 @@ async def _stream_audio_loop(self) -> None: audio_source = await decode_audio(self.source, source_format=self.source_format) fmt = audio_source.format - play_start_us = int(time.monotonic() * 1_000_000) + DEFAULT_INITIAL_DELAY_US + play_start_us = server_now_us() + DEFAULT_INITIAL_DELAY_US async for pcm_chunk in audio_source.generator: if self._shutdown_requested: @@ -278,7 +296,7 @@ async def _stream_audio_loop(self) -> None: play_start_us += chunk_duration_us - now_us = int(time.monotonic() * 1_000_000) + now_us = server_now_us() ahead_us = play_start_us - DEFAULT_INITIAL_DELAY_US - now_us if ahead_us > MAX_BUFFER_AHEAD_US: await asyncio.sleep((ahead_us - MAX_BUFFER_AHEAD_US) / 1_000_000) From 0125ce1800280426f528bd5ef4e14f2e84a9697b Mon Sep 17 00:00:00 2001 From: Aleksandr Kadykov Date: Sun, 4 Oct 2026 12:49:52 +0000 Subject: [PATCH 02/15] docs: clarify daemon upgrade flow for dedicated sendspin service user (#281) When we install the CLI as a system service, the script proposes to install it as a dedicated user. However, in the current documentation, we just provide the instructions to update the package if it was installed as a current user, but not as a dedicated user. This pull request just adds explicit instructions for this case. --- README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/README.md b/README.md index ecee88f..2905434 100644 --- a/README.md +++ b/README.md @@ -85,6 +85,13 @@ uv tool upgrade sendspin sudo systemctl restart sendspin ``` +The installer may create a dedicated `sendspin` service account. In that case, the package is installed in that user's `uv` environment, not in your own shell. Upgrade the package as that user instead: + +```bash +sudo -u sendspin bash -l -c "uv tool upgrade sendspin" +sudo systemctl restart sendspin +``` + Your client name, audio device selection, and other settings in `~/.config/sendspin/settings-daemon.json` are preserved during the update. > **Note:** You do **not** need to uninstall and reinstall when updating. Your configuration (client name, audio device, delay settings) is stored separately and will be preserved. From 6c73885ef94a30152f437fb69da9a74e32c15694 Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Tue, 6 Oct 2026 19:59:56 +1300 Subject: [PATCH 03/15] fix(daemon): keep reconnecting after unexpected errors instead of exiting 0 (#284) ## Problem In `Daemon._connection_loop` (`sendspin/daemon/daemon.py`), any non-network exception hits `except Exception: ... break`. The break returns through `_run_client_initiated` into `run()`'s `finally`, which ends with `return 0` -- so the daemon exits successfully, and the shipped systemd unit (`Restart=on-failure`) never restarts it. One transient unexpected error = silent death reported as success. ## Fix Treats unexpected errors like connection errors: log (now including the exception type) and retry with the same capped backoff instead of breaking out of the loop. Cancellation still propagates via `CancelledError`. ## Verification - `pytest tests/daemon/test_daemon.py`: 3 passed - `ruff check` clean If you would rather fail loudly than retry forever on deterministic errors, an alternative is keeping the break but returning non-zero -- glad to switch to that if preferred. --- sendspin/daemon/daemon.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/sendspin/daemon/daemon.py b/sendspin/daemon/daemon.py index 09dffda..f65a43b 100644 --- a/sendspin/daemon/daemon.py +++ b/sendspin/daemon/daemon.py @@ -14,6 +14,7 @@ from aiosendspin.client import ClientListener, PairingSupport, SendspinClient from aiosendspin.models.core import GroupUpdateServerPayload, ServerCommandPayload from aiosendspin.models.player import ClientHelloPlayerSupport, SupportedAudioFormat +from aiosendspin.noise.driver import HandshakeAbortedError from aiosendspin_mpris import MPRIS_AVAILABLE, SendspinMpris from aiosendspin.models.types import ( Activity, @@ -433,7 +434,21 @@ async def _connection_loop(self, url: str) -> None: logger.info("Reconnecting to %s", url) - except (TimeoutError, OSError, ClientError) as e: + except ( + TimeoutError, + OSError, + ClientError, + RuntimeError, + HandshakeAbortedError, + ) as e: + # Retry only the connection-failure set aiosendspin's own + # client uses (HandshakeAbortedError, OSError, RuntimeError, + # TimeoutError). RuntimeError covers "server activation + # rejected" / "connection closed before admission". Anything + # outside this set is a genuine bug and is left to propagate, + # so the daemon exits non-zero and the shipped systemd unit + # (Restart=on-failure) restarts it instead of the process + # dying silently with exit code 0. logger.warning( "Connection error (%s), retrying in %.0fs", type(e).__name__, @@ -443,10 +458,6 @@ async def _connection_loop(self, url: str) -> None: await asyncio.sleep(error_backoff) error_backoff = min(error_backoff * 2, max_backoff) - except Exception: - logger.exception("Unexpected error during connection") - break - def _handle_server_command(self, payload: ServerCommandPayload) -> None: """Handle server commands for player volume/mute control.""" if payload.player is None or self._settings is None: From 46a9c4946a40f142c0220280124b090633810d14 Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Tue, 6 Oct 2026 20:01:47 +1300 Subject: [PATCH 04/15] fix(tui): idle the visualizer refresh once interpolation settles (#288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `VisualizerState.is_active` returns `bool(self._spectrum_target)`, which stays `True` for as long as any target data exists — it never returns to rest. `SendspinUI._needs_visualizer_refresh()` gates its periodic refresh on that property, so once playback pauses and no further frames arrive the target list is never emptied and the whole layout keeps re-rendering at `VISUALIZER_REFRESH_INTERVAL` (16 ms) indefinitely. `BeatState.is_active` already returns to rest once its pulse decays — the spectrum state was the odd one out. ## Fix Report active only while the smoothed spectrum, peaks and loudness have not yet settled (within a small epsilon). Once interpolation and the peak decay finish, `is_active` is `False` and the refresh loop idles; the next frame marks the UI dirty again and restarts it. This keeps the property's documented meaning — "pending visualizer data to animate" — and needs no timer or idle timeout. ## Verification - New tests in `tests/tui/test_visualizer.py`: - idle with no data; - after a frame, `is_active` is `True` and returns `False` once stepping has settled the values (this failed before the change). - `pytest` — 149 passed; `ruff check`, `ruff format --check`, `mypy sendspin` clean. --- sendspin/tui/visualizer.py | 27 +++++++++++++++++++++++++-- tests/tui/test_visualizer.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/sendspin/tui/visualizer.py b/sendspin/tui/visualizer.py index cd9c78a..2199b47 100644 --- a/sendspin/tui/visualizer.py +++ b/sendspin/tui/visualizer.py @@ -37,6 +37,10 @@ class PeakEvent: # Interpolation response speed in units per second. _SMOOTH_RATE_PER_SECOND = 14.0 +# Values closer than this are treated as settled, letting the periodic refresh +# loop idle once a frame's interpolation and the peak decay have finished. +_SETTLE_EPSILON = 1e-3 + # Peak hold configuration _PEAK_HOLD_SECONDS = 0.5 _PEAK_FALL_RATE = 0.375 # normalized units per second (≈6 rows/sec at 16 rows) @@ -222,8 +226,27 @@ def _step(self) -> None: @property def is_active(self) -> bool: - """Whether there is pending visualizer data to animate.""" - return bool(self._spectrum_target) + """Whether the visualizer is still animating toward its target. + + True only while the smoothed values have not settled, so the refresh + loop idles once interpolation and the peak decay finish -- including + when playback pauses and no further frames arrive. + """ + if len(self._spectrum) != len(self._spectrum_target): + return True + if len(self._peaks) != len(self._spectrum): + return True + if abs(self._loudness - self._loudness_target) > _SETTLE_EPSILON: + return True + if any( + abs(current - target) > _SETTLE_EPSILON + for current, target in zip(self._spectrum, self._spectrum_target, strict=True) + ): + return True + return any( + abs(peak - bar) > _SETTLE_EPSILON + for peak, bar in zip(self._peaks, self._spectrum, strict=True) + ) def step(self) -> None: """Advance displayed values toward targets. diff --git a/tests/tui/test_visualizer.py b/tests/tui/test_visualizer.py index 1c4f962..71f7b83 100644 --- a/tests/tui/test_visualizer.py +++ b/tests/tui/test_visualizer.py @@ -544,3 +544,31 @@ def test_pitch_arrow_on_separate_line_below_f_peak() -> None: pitch_row = next(i for i, row in enumerate(rows) if "▲" in row.plain) assert f_peak_row < pitch_row assert "▲" not in rows[f_peak_row].plain + + +# --- VisualizerState activity tests --- + + +def test_visualizer_state_idle_without_data() -> None: + state = VisualizerState() + assert state.is_active is False + + +def test_visualizer_state_settles_to_idle_after_frame() -> None: + """is_active goes False once interpolation settles, so the refresh loop idles. + + Previously this stayed True forever because the target list was non-empty, + so a paused stream kept the whole layout re-rendering at the frame interval. + """ + state = VisualizerState() + state.update([65535, 65535], loudness=65535) + assert state.is_active is True + + with patch("sendspin.tui.visualizer.time") as mock_time: + base = time.monotonic() + for i in range(1, 200): + mock_time.monotonic.return_value = base + i * 0.05 + state.step() + + assert state.is_active is False + assert state.get_spectrum() == [1.0, 1.0] From bf9abb576b7b019a859f6adccf79aeb1b5d898de Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Tue, 6 Oct 2026 20:03:56 +1300 Subject: [PATCH 05/15] fix(settings): create the identity key file with owner-only mode from the outset (#287) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `_load_identity()` persists the generated Noise identity private key with: ```python path.write_text(json.dumps({"private_key": b64url_encode(identity.private_bytes)})) os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) ``` The file is created with the process umask (commonly `0644`) and only then restricted, so the private key is briefly readable by group/other — a write-then-chmod (TOCTOU) window. This key is the client's cryptographic identity and must stay stable across restarts, so an exposure is worth avoiding. ## Fix Create the file with `os.open(..., 0o600)` and write through the returned descriptor, so the restrictive mode applies atomically at creation. `0600` has no group/other bits for the umask to clear, so the result is owner-only on any normal umask. ## Verification - New `tests/test_settings.py`: - the generated key file has no group/other permission bits (`st_mode & 0o077 == 0`); - the key still round-trips to the same `peer_id` on reload. - `pytest` — 148 passed; `ruff check`, `ruff format --check`, `mypy sendspin` clean. --- sendspin/settings.py | 8 ++++++-- tests/test_settings.py | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 tests/test_settings.py diff --git a/sendspin/settings.py b/sendspin/settings.py index 04a6ca5..159fe63 100644 --- a/sendspin/settings.py +++ b/sendspin/settings.py @@ -353,8 +353,12 @@ def _load_identity(path: Path, settings_path: Path) -> Identity: _warn_if_upgrading_from_legacy_client_id(settings_path) identity = Identity.generate() path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(json.dumps({"private_key": b64url_encode(identity.private_bytes)})) - os.chmod(path, stat.S_IRUSR | stat.S_IWUSR) + # Create the key file with owner-only permissions from the outset: + # writing first and chmod-ing after leaves a window where the private + # key is readable by other users under a permissive umask. + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, stat.S_IRUSR | stat.S_IWUSR) + with os.fdopen(fd, "w", encoding="utf-8") as handle: + handle.write(json.dumps({"private_key": b64url_encode(identity.private_bytes)})) return identity diff --git a/tests/test_settings.py b/tests/test_settings.py new file mode 100644 index 0000000..4b97ad1 --- /dev/null +++ b/tests/test_settings.py @@ -0,0 +1,22 @@ +"""Tests for settings persistence.""" + +from __future__ import annotations + +from pathlib import Path + +from sendspin.settings import _load_identity + + +def test_load_identity_creates_key_file_without_group_or_other_access(tmp_path: Path) -> None: + """A generated private key must never be readable by group or other.""" + path = tmp_path / "identity-tui.json" + settings_path = tmp_path / "settings-tui.json" + + identity = _load_identity(path, settings_path) + + assert path.exists() + assert path.stat().st_mode & 0o077 == 0 + + # The persisted key must still round-trip to the same client identity. + reloaded = _load_identity(path, settings_path) + assert reloaded.peer_id == identity.peer_id From 2139ed7483df6fcaeb596e0531ce2ac8cf0c6ed9 Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Tue, 6 Oct 2026 20:04:58 +1300 Subject: [PATCH 06/15] fix(utils): log fire-and-forget background task failures (#283) ## Problem `create_task()` in `sendspin/utils.py` registers a done-callback that retrieves the exception (suppressing "never retrieved" warnings) and then discards it with no logging. Every fire-and-forget failure in the daemon is invisible, including the external-volume path (hook/amixer/PulseAudio errors swallowed, volume never published). ## Fix Replaces the lambda with a `_log_task_exception` callback that logs the exception with the task name (`exc_info` preserved). Cancelled tasks stay silent, as before; behaviour is otherwise unchanged. ## Verification - Full suite (`pytest tests/`, excluding serve + audio_devices): 123 passed - `ruff check` clean Small, no behaviour change beyond visibility. Happy to lower it to debug level if error is too noisy. --- sendspin/utils.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/sendspin/utils.py b/sendspin/utils.py index d259ed8..c4dace3 100644 --- a/sendspin/utils.py +++ b/sendspin/utils.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import logging import platform import sys import uuid @@ -13,6 +14,8 @@ from aiosendspin.models.core import DeviceInfo +logger = logging.getLogger(__name__) + _T = TypeVar("_T") # Check if eager_start is supported (Python 3.12+) @@ -61,11 +64,25 @@ def create_task( TASKS.add(task) task.add_done_callback(TASKS.discard) - task.add_done_callback(lambda t: t.exception() if not t.cancelled() else None) + task.add_done_callback(_log_task_exception) return task +def _log_task_exception(task: asyncio.Task[Any]) -> None: + """Log a fire-and-forget background task failure instead of swallowing it. + + Retrieving the exception suppresses "exception was never retrieved" + warnings, but the previous lambda then discarded it — every background + failure (hook/amixer/PulseAudio volume errors included) was invisible. + """ + if task.cancelled(): + return + exc = task.exception() + if exc is not None: + logger.error("Background task %r failed", task.get_name(), exc_info=exc) + + def _detect_mac_address() -> str | None: """Return a stable hardware MAC address, or None if only a synthesized one is available. From 40d2b4dea01111314351e4e65bcacb715850b723 Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Tue, 6 Oct 2026 20:05:24 +1300 Subject: [PATCH 07/15] fix(settings): write settings file atomically via temp + rename (#285) ## Problem `BaseSettings._save` (`sendspin/settings.py`, shared by client and serve settings) rewrites the live JSON file in place with `write_text`. A kill mid-write truncates it, and on next start the `JSONDecodeError` handler logs a warning and silently falls back to defaults -- volume, name, interface and hooks all revert with no obvious cause. ## Fix Write-temp-then-rename: content goes to a `.tmp` sibling, is fsynced, then `os.replace()` swaps it into place atomically. A crash can now only leave the previous good file behind. Same `OSError` handling as before. ## Verification - Full suite (`pytest tests/`, excluding serve + audio_devices): 123 passed - `ruff check` clean Covers both `ClientSettings` and `ServeSettings` since they share `_save`. --- sendspin/settings.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sendspin/settings.py b/sendspin/settings.py index 159fe63..0b5e9e6 100644 --- a/sendspin/settings.py +++ b/sendspin/settings.py @@ -105,7 +105,13 @@ def _save(self) -> None: return try: self._settings_file.parent.mkdir(parents=True, exist_ok=True) - self._settings_file.write_text(json.dumps(self.to_dict(), indent=2) + "\n") + # Write atomically: a kill mid-write must not truncate the live + # file (a truncated file falls back to defaults on next load). + tmp_file = self._settings_file.with_suffix(self._settings_file.suffix + ".tmp") + tmp_file.write_text(json.dumps(self.to_dict(), indent=2) + "\n") + with open(tmp_file, "rb") as f: + os.fsync(f.fileno()) + os.replace(tmp_file, self._settings_file) logger.debug("Saved settings to %s", self._settings_file) except OSError as e: logger.warning("Failed to save settings to %s: %s", self._settings_file, e) From 2a1dbbe1f682f57afc88f4cea148b8d7b8294e31 Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Tue, 6 Oct 2026 20:06:33 +1300 Subject: [PATCH 08/15] fix(audio): treat ValueError from format probing as unsupported (raw ALSA devices) (#286) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `--audio-device` accepts raw ALSA names (e.g. `hw:CARD=sndrpihifiberry,DEV=0`, `dmix`, `plug`). `resolve_audio_device()` handles those through `_try_alsa_device()`, which — since #224 — treats PortAudio's `ValueError` as a lookup miss and validates the name against `aplay -L`. The format-detection step that runs afterwards was missed: `_check_format()` only catches `sounddevice.PortAudioError`, while PortAudio raises `ValueError` for the very same unrecognised names. A device resolved this way has `index=None` and only an `alsa_device_name`, so `device.device_id` is that raw name; `check_output_settings()` therefore raises `ValueError`, it escapes `_check_format()`, and it propagates out of `detect_supported_audio_formats()` — the client fails at startup before it ever reaches the server. That is the failure reported in #262 (and the same class as #251): a device listed by `sendspin audio-devices list` still cannot be used. ## Fix Catch `ValueError` alongside `PortAudioError` in `_check_format()` and report the format as unsupported, exactly as `_try_alsa_device()` already does on the resolution path. `detect_supported_audio_formats()` then takes its existing "Could not detect supported formats, using safe defaults" branch, so startup proceeds and the real format is negotiated with the server later. ## Verification - `pytest` — 149 passed, including 2 new tests in `tests/test_audio_devices.py`: - `_check_format()` returns `False` (instead of raising) when `check_output_settings` raises `ValueError`; - `detect_supported_audio_formats()` returns non-empty safe defaults for an ALSA-only device. - `ruff check`, `ruff format --check`, `mypy sendspin` all clean. Fixes #262 Refs #251 --- sendspin/audio_devices.py | 7 ++++++- tests/test_audio_devices.py | 41 ++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/sendspin/audio_devices.py b/sendspin/audio_devices.py index a365188..eb1bf94 100644 --- a/sendspin/audio_devices.py +++ b/sendspin/audio_devices.py @@ -90,7 +90,12 @@ def _check_format(device: AudioDevice, rate: int, channels: int, dtype: str) -> dtype=dtype, ) return True - except sounddevice.PortAudioError: + except (sounddevice.PortAudioError, ValueError): + # PortAudio raises ValueError (not PortAudioError) for device names it + # does not recognise, e.g. raw ALSA names such as hw:CARD=...,DEV=... . + # The device itself is valid -- _try_alsa_device() already accepted it -- + # so treat an unprobeable format as unsupported here and let the caller + # fall back to its safe defaults instead of failing at startup. return False diff --git a/tests/test_audio_devices.py b/tests/test_audio_devices.py index dc52e42..7db483c 100644 --- a/tests/test_audio_devices.py +++ b/tests/test_audio_devices.py @@ -7,7 +7,12 @@ import sounddevice import sendspin.audio_devices as _mod -from sendspin.audio_devices import _try_alsa_device +from sendspin.audio_devices import ( + AudioDevice, + _check_format, + _try_alsa_device, + detect_supported_audio_formats, +) def test_try_alsa_device_returns_device_when_portaudio_accepts(): @@ -118,3 +123,37 @@ def test_try_alsa_device_accepts_alsa_only_device(): assert result is not None assert result.alsa_device_name == "bluealsa" + + +def _alsa_only_device() -> AudioDevice: + """A device resolved by name that PortAudio cannot resolve itself.""" + return AudioDevice( + index=None, + name="hw:CARD=sndrpihifiberry,DEV=0", + output_channels=2, + sample_rate=48000.0, + is_default=False, + alsa_device_name="hw:CARD=sndrpihifiberry,DEV=0", + ) + + +def test_check_format_reports_unsupported_for_unknown_device_name(): + """PortAudio raises ValueError for raw ALSA names; treat it as unsupported, not fatal.""" + with patch.object( + sounddevice, + "check_output_settings", + side_effect=ValueError("No output device matching"), + ): + assert _check_format(_alsa_only_device(), 48000, 2, "int16") is False + + +def test_detect_supported_audio_formats_falls_back_for_alsa_only_device(): + """Startup must not raise for a device PortAudio cannot probe.""" + with patch.object( + sounddevice, + "check_output_settings", + side_effect=ValueError("No output device matching"), + ): + formats = detect_supported_audio_formats(_alsa_only_device()) + + assert formats, "expected safe-default formats for an unprobeable device" From 97b628c1e8bca9b19697ae6638fb871eeedfc77b Mon Sep 17 00:00:00 2001 From: Maxim Raznatovski Date: Mon, 12 Oct 2026 06:58:06 +0200 Subject: [PATCH 09/15] Auto-label PRs and sort all labels into release notes (#292) # What does this implement/fix? Ports the PR label setup from `aiosendspin`. The labels in the repo, the release notes sections and the version resolver didn't match. So `new-feature` PRs bumped the minor version but ended up outside every release notes section, `refactor` and `documentation` PRs never got a section, and `ci` was listed twice without existing as a label. Most recent PRs also had no label at all. Now `release-drafter.yml` matches `aiosendspin`, and a new PR template has one checkbox per label, which the `pr-labels.yaml` workflow applies. I already added the `ci` label and updated the label descriptions. The workflow runs on `pull_request_target`, which always uses the version on `main`, so it applies to every PR once this is merged. Open PRs will need a ticked box on their next update. The workflow is ported from: - https://github.com/Sendspin/aiosendspin/pull/450 **Related issue (if applicable):** - ## Types of changes - [ ] Bugfix (non-breaking change which fixes an issue): `bugfix` - [ ] New feature (non-breaking change which adds functionality): `new-feature` - [ ] Enhancement to an existing feature: `enhancement` - [ ] Breaking change (changes CLI options, settings, hooks or the protocol in a way that is not backwards compatible): `breaking-change` - [ ] Refactor (no behavior change): `refactor` - [ ] Documentation only: `documentation` - [ ] Maintenance / chore: `maintenance` - [x] CI / workflow change: `ci` - [ ] Dependencies bump: `dependencies` --- .github/PULL_REQUEST_TEMPLATE.md | 24 ++++++++ .github/release-drafter.yml | 7 ++- .github/workflows/pr-labels.yaml | 95 ++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 .github/workflows/pr-labels.yaml diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..c7adfad --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,24 @@ +# What does this implement/fix? + + + +**Related issue (if applicable):** + +- + +## Types of changes + + + +- [ ] Bugfix (non-breaking change which fixes an issue): `bugfix` +- [ ] New feature (non-breaking change which adds functionality): `new-feature` +- [ ] Enhancement to an existing feature: `enhancement` +- [ ] Breaking change (changes CLI options, settings, hooks or the protocol in a way that is not backwards compatible): `breaking-change` +- [ ] Refactor (no behavior change): `refactor` +- [ ] Documentation only: `documentation` +- [ ] Maintenance / chore: `maintenance` +- [ ] CI / workflow change: `ci` +- [ ] Dependencies bump: `dependencies` diff --git a/.github/release-drafter.yml b/.github/release-drafter.yml index c4d0f60..4fbf616 100644 --- a/.github/release-drafter.yml +++ b/.github/release-drafter.yml @@ -12,19 +12,20 @@ categories: - title: "🚀 Features" labels: - - "feature" + - "new-feature" - "enhancement" - title: "🧰 Maintenance" labels: + - "refactor" + - "documentation" - "ci" - "maintenance" - - title: "⬆️ Dependencies and CI" + - title: "⬆️ Dependencies" collapse-after: 1 labels: - "dependencies" - - "ci" template: | ## What’s Changed diff --git a/.github/workflows/pr-labels.yaml b/.github/workflows/pr-labels.yaml new file mode 100644 index 0000000..fe31b79 --- /dev/null +++ b/.github/workflows/pr-labels.yaml @@ -0,0 +1,95 @@ +--- +name: PR Labels + +# Each PR must carry exactly one of the labels the release notes sort by. +# The label comes from the ticked "Types of changes" checkbox in the PR +# template, since external contributors can't label their own PRs. + +# yamllint disable-line rule:truthy +on: + pull_request_target: + types: + - opened + - edited + - synchronize + - labeled + - unlabeled + - reopened + - ready_for_review + branches: + - main + +permissions: + pull-requests: write + +jobs: + pr_labels: + name: Verify + runs-on: ubuntu-latest + # Bots like Dependabot don't use the template and label their own PRs. + if: github.event.pull_request.user.type != 'Bot' + steps: + - name: 🏷 Apply label from PR description checkbox + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const known = [ + 'breaking-change', 'bugfix', 'new-feature', 'enhancement', + 'refactor', 'documentation', 'maintenance', 'ci', 'dependencies', + ]; + + const { owner, repo } = context.repo; + const issue_number = context.payload.pull_request.number; + const current = (context.payload.pull_request.labels || []) + .map(l => l.name); + + // Match a ticked bullet whose first backticked word is a label, + // e.g. - [x] CI / workflow change: `ci` + const body = context.payload.pull_request.body || ''; + const re = /^\s*-\s*\[x\][^`\n]*`([^`]+)`/gim; + const ticked = []; + for (const m of body.matchAll(re)) { + const name = m[1].trim(); + if (known.includes(name) && !ticked.includes(name)) { + ticked.push(name); + } + } + + if (ticked.length > 1) { + core.setFailed( + `Multiple "Types of changes" checkboxes are ticked ` + + `(${ticked.join(', ')}). Tick exactly one.` + ); + return; + } + + // A ticked box wins. Without one, accept a single known label + // already on the PR, for PRs opened without the template. + if (ticked.length === 0) { + const existing = current.filter(n => known.includes(n)); + if (existing.length === 1) { + core.info(`Skipping: PR already labelled '${existing[0]}'.`); + return; + } + core.setFailed( + 'No "Types of changes" checkbox is ticked in the PR ' + + 'description. Edit the description and tick exactly ' + + 'one box so this PR can be release-noted.' + ); + return; + } + + const desired = ticked[0]; + + for (const name of current) { + if (known.includes(name) && name !== desired) { + await github.rest.issues.removeLabel({ + owner, repo, issue_number, name, + }).catch(() => {}); + } + } + if (!current.includes(desired)) { + await github.rest.issues.addLabels({ + owner, repo, issue_number, labels: [desired], + }); + } From 0d8c33536a3af7fe9a51f79a7b3528024c872f25 Mon Sep 17 00:00:00 2001 From: Maxim Raznatovski Date: Mon, 12 Oct 2026 07:08:58 +0200 Subject: [PATCH 10/15] Add `--unpaired-access` option, on by default (#293) # What does this implement/fix? Adds `--unpaired-access` and an `unpaired_access` setting, so servers can play on this client without pairing. It's on by default, since the CLI has no audio input role and Music Assistant approves unpaired clients automatically. This way upgraded headless players don't need someone to catch the pairing PIN in the daemon log. Installs that already have a pairing file start accepting unpaired servers after this. `--unpaired-access false` keeps requiring pairing. **Related issue (if applicable):** ## Types of changes - [ ] Bugfix (non-breaking change which fixes an issue): `bugfix` - [x] New feature (non-breaking change which adds functionality): `new-feature` - [ ] Enhancement to an existing feature: `enhancement` - [ ] Breaking change (changes CLI options, settings, hooks or the protocol in a way that is not backwards compatible): `breaking-change` - [ ] Refactor (no behavior change): `refactor` - [ ] Documentation only: `documentation` - [ ] Maintenance / chore: `maintenance` - [ ] CI / workflow change: `ci` - [ ] Dependencies bump: `dependencies` --- README.md | 14 +++++++++++--- sendspin/cli.py | 26 ++++++++++++++++++++++++++ sendspin/settings.py | 2 ++ tests/test_settings.py | 19 ++++++++++++++++++- 4 files changed, 57 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 2905434..64869b6 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,7 @@ Settings are stored in `~/.config/sendspin/`: "listen_port": 8927, "use_mpris": true, "use_hardware_volume": true, + "unpaired_access": true, "hook_set_volume": "/usr/local/bin/set-avr-volume", "manufacturer": "Acme Corp", "product_name": "Living Room Speaker", @@ -154,6 +155,7 @@ Settings are stored in `~/.config/sendspin/`: | `listen_port` | integer | daemon/serve | Listen port (`--port`, default: 8927) | | `use_mpris` | boolean | TUI/daemon | Enable MPRIS integration (default: true) | | `use_hardware_volume` | boolean | TUI/daemon | Control hardware/system output volume instead of software volume (`--hardware-volume true/false`). Default: on for daemon (if available), off for TUI | +| `unpaired_access` | boolean | TUI/daemon | Allow servers to play on this client without pairing (`--unpaired-access true/false`, default: true). See [Pairing](#pairing) | | `hook_set_volume` | string | TUI/daemon | Script to run for external volume control (`--hook-set-volume`). Receives the effective volume 0-100 as the last argument | | `hook_start` | string | TUI/daemon | Command to run when audio stream starts | | `hook_stop` | string | TUI/daemon | Command to run when audio stream stops | @@ -192,13 +194,19 @@ same server don't require pairing again. pairing completes. - **Daemon**: the PIN is only logged (`Pairing required: enter PIN ...`) — there's no other surface for it. You need to be watching the daemon's output at the moment it first - connects to a new server. Under systemd, that means `journalctl -u sendspin -f`. There's - no unattended-pairing option today (no PIN written to a file, no QR code); for a headless - install, plan to watch the log during first setup for each new server. + connects to a new server. Under systemd, that means `journalctl -u sendspin -f`. For a + headless install with unpaired access turned off, plan to watch the log during first setup + for each new server. If a pairing attempt fails (e.g. a mismatched PIN), the client logs the reason and you can retry by reconnecting. +Unpaired access is on by default, so servers can play on this client without a PIN once +the server approves it (Music Assistant does this automatically). Unpaired connections are +still encrypted, but another device on the network could impersonate either side. To +require pairing, use `--unpaired-access false` (or `"unpaired_access": false` in the +settings file). + **Upgrading from a version before pairing support:** this client's protocol identity (`client_id`) used to be a stable, often user-chosen string; it's now derived from a generated cryptographic identity instead, so upgrading a previously-configured install diff --git a/sendspin/cli.py b/sendspin/cli.py index bc06d57..7620624 100644 --- a/sendspin/cli.py +++ b/sendspin/cli.py @@ -10,6 +10,7 @@ import sys import traceback from collections.abc import Sequence +from dataclasses import replace from importlib.metadata import version from typing import TYPE_CHECKING, Any, Literal, Protocol @@ -216,6 +217,13 @@ def _add_player_runtime_options(target: ArgumentTarget, *, suppress_defaults: bo metavar="{true,false}", help="Enable or disable hardware/system volume control (daemon: on, TUI: off)", ) + target.add_argument( + "--unpaired-access", + default=default, + type=arg_str_to_bool, + metavar="{true,false}", + help="Allow servers to play on this client without pairing (default: true)", + ) target.add_argument( "--manufacturer", type=str, @@ -452,6 +460,13 @@ def _build_parser() -> argparse.ArgumentParser: metavar="{true,false}", help="Enable or disable hardware/system volume control (daemon: on, TUI: off)", ) + daemon_parser.add_argument( + "--unpaired-access", + default=None, + type=arg_str_to_bool, + metavar="{true,false}", + help="Allow servers to play on this client without pairing (default: true)", + ) daemon_parser.add_argument( "--hook-start", type=str, @@ -865,6 +880,10 @@ async def _run_client_mode(args: argparse.Namespace) -> int: args.product_name = settings.product_name if args.interface is None: args.interface = settings.interface + if args.unpaired_access is None: + args.unpaired_access = settings.unpaired_access + if not isinstance(args.unpaired_access, bool): + raise CLIError("unpaired_access in the settings file must be true or false", 1) # Set up logging: daemon uses stderr, TUI writes to sendspin.log # so log output doesn't interfere with the Rich display. @@ -878,6 +897,13 @@ async def _run_client_mode(args: argparse.Namespace) -> int: handler.setFormatter(logging.Formatter("%(asctime)s %(levelname)s %(name)s: %(message)s")) logging.basicConfig(level=log_level, handlers=[handler]) + pairing_config = await pairing_store.get_pairing_config() + if pairing_config.unpaired_access_enabled != args.unpaired_access: + await pairing_store.store_pairing_config( + replace(pairing_config, unpaired_access_enabled=args.unpaired_access) + ) + LOGGER.info("Unpaired access %s", "enabled" if args.unpaired_access else "disabled") + audio_device = resolve_audio_device(args.audio_device) volume_controller: VolumeController | None = None diff --git a/sendspin/settings.py b/sendspin/settings.py index 0b5e9e6..582c056 100644 --- a/sendspin/settings.py +++ b/sendspin/settings.py @@ -129,6 +129,7 @@ class ClientSettings(BaseSettings): use_mpris: bool = True audio_format: str | None = None use_hardware_volume: bool | None = None + unpaired_access: bool = True hook_set_volume: str | None = None hook_start: str | None = None hook_stop: str | None = None @@ -228,6 +229,7 @@ def _load(self) -> bool: self.use_mpris = data.get("use_mpris", True) self.audio_format = data.get("audio_format") self.use_hardware_volume = data.get("use_hardware_volume") + self.unpaired_access = data.get("unpaired_access", True) self.hook_set_volume = data.get("hook_set_volume") self.hook_start = data.get("hook_start") self.hook_stop = data.get("hook_stop") diff --git a/tests/test_settings.py b/tests/test_settings.py index 4b97ad1..d18ae77 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -2,9 +2,12 @@ from __future__ import annotations +import json from pathlib import Path -from sendspin.settings import _load_identity +import pytest + +from sendspin.settings import _load_identity, get_client_settings def test_load_identity_creates_key_file_without_group_or_other_access(tmp_path: Path) -> None: @@ -20,3 +23,17 @@ def test_load_identity_creates_key_file_without_group_or_other_access(tmp_path: # The persisted key must still round-trip to the same client identity. reloaded = _load_identity(path, settings_path) assert reloaded.peer_id == identity.peer_id + + +@pytest.mark.parametrize( + ("stored", "expected"), + [({"name": "Kitchen"}, True), ({"unpaired_access": False}, False)], +) +async def test_unpaired_access_from_settings_file( + tmp_path: Path, stored: dict[str, object], expected: bool +) -> None: + (tmp_path / "settings-daemon.json").write_text(json.dumps(stored)) + + settings = await get_client_settings("daemon", str(tmp_path)) + + assert settings.unpaired_access is expected From 6689c37090f2f3622b94a7b3c30be2d44d20b1bc Mon Sep 17 00:00:00 2001 From: Tucker Kern Date: Sat, 17 Oct 2026 07:48:48 -0600 Subject: [PATCH 11/15] Fix logging error in CLI when ALSA card is string instead of int (#295) # What does this implement/fix? Fixes logging error in CLI when ALSA card is a string not an int. **Related issue (if applicable):** - Close #294 ## Types of changes - [x] Bugfix (non-breaking change which fixes an issue): `bugfix` - [ ] New feature (non-breaking change which adds functionality): `new-feature` - [ ] Enhancement to an existing feature: `enhancement` - [ ] Breaking change (changes CLI options, settings, hooks or the protocol in a way that is not backwards compatible): `breaking-change` - [ ] Refactor (no behavior change): `refactor` - [ ] Documentation only: `documentation` - [ ] Maintenance / chore: `maintenance` - [ ] CI / workflow change: `ci` - [ ] Dependencies bump: `dependencies` --- sendspin/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sendspin/cli.py b/sendspin/cli.py index 7620624..a137797 100644 --- a/sendspin/cli.py +++ b/sendspin/cli.py @@ -916,7 +916,7 @@ async def _run_client_mode(args: argparse.Namespace) -> int: if alsa_info is not None: card, element = alsa_info LOGGER.info( - "Using ALSA mixer volume control: card %d, element %r", + "Using ALSA mixer volume control: card %r, element %r", card, element, ) From c5e9eb86a1e515ca58c829aa118510d99d876734 Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Sun, 18 Oct 2026 02:50:57 +1300 Subject: [PATCH 12/15] fix(daemon): log repeated admission timeouts at debug, first at info (#297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # What does this implement/fix? When a server keeps redialing, every unadmitted connection logs the same info line, so the daemon log fills with repeats of one event: INFO:sendspin.daemon.daemon:Incoming connection did not complete admission (handshake timed out or was aborted); waiting for a retry. I hit this watching my own player logs, where the line repeats on every reconnect and buries everything else. This keeps the first occurrence at info (an unadmitted connection would otherwise be fully silent, as the code comment notes) and moves repeats to debug. The flag resets once a connection is admitted, so a later wedged spell is visible again at info. No behavior change apart from log level: three lines of state (one bool, set on first log, cleared on admission). **Related issue (if applicable):** - none filed — happy to open one if you would rather track it that way. ## Types of changes - [x] Bugfix (non-breaking change which fixes an issue): `bugfix` - [ ] New feature (non-breaking change which adds functionality): `new-feature` - [ ] Enhancement to an existing feature: `enhancement` - [ ] Breaking change (changes CLI options, settings, hooks or the protocol in a way that is not backwards compatible): `breaking-change` - [ ] Refactor (no behavior change): `refactor` - [ ] Documentation only: `documentation` - [ ] Maintenance / chore: `maintenance` - [ ] CI / workflow change: `ci` - [ ] Dependencies bump: `dependencies` --- sendspin/daemon/daemon.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/sendspin/daemon/daemon.py b/sendspin/daemon/daemon.py index f65a43b..640be74 100644 --- a/sendspin/daemon/daemon.py +++ b/sendspin/daemon/daemon.py @@ -84,6 +84,7 @@ def __init__(self, args: DaemonArgs) -> None: self._static_delay_ms: float = 0.0 self._connection_lock: asyncio.Lock | None = None self._server_url: str | None = None + self._admission_timeout_logged: bool = False self._group_update_unsubscribe: Callable[[], None] | None = None self._server_command_unsubscribe: Callable[[], None] | None = None self._pairing_abort_unsubscribe: Callable[[], None] | None = None @@ -356,12 +357,27 @@ async def _handle_server_connection(self, ws: web.WebSocketResponse) -> None: if exc is not None: logger.warning("Handshake with server failed: %s", exc) else: - logger.info( - "Incoming connection did not complete admission " - "(handshake timed out or was aborted); waiting for a retry." - ) + # Admission timeouts recur on every reconnect while a server + # keeps redialing, so log the first at info (an unadmitted + # connection would otherwise be fully silent) and repeats at + # debug. The flag resets below once a connection is admitted. + if not self._admission_timeout_logged: + logger.info( + "Incoming connection did not complete admission " + "(handshake timed out or was aborted); waiting for a retry." + ) + self._admission_timeout_logged = True + else: + logger.debug( + "Incoming connection did not complete admission " + "(handshake timed out or was aborted); waiting for a retry." + ) return + # Reaching here means the connection was admitted: re-arm the + # first-time info log so a later wedged spell is visible again. + self._admission_timeout_logged = False + # Lock ensures we wait for any in-progress handshake to complete # before disconnecting the previous server async with self._connection_lock: From 004a88b2f2617eebed6baae10a3222f506682fe9 Mon Sep 17 00:00:00 2001 From: kiwipaulrob Date: Sun, 25 Oct 2026 05:10:41 +1300 Subject: [PATCH 13/15] fix(tui): clear now-playing metadata on a null metadata payload (#289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `_handle_metadata_update()` treats a `metadata: null` payload as a no-op: ```python if payload.metadata is None or not state.update_metadata(payload.metadata): return ``` But `None` is a spec-legal *clear*: an absent field arrives as `UndefinedField` (handled one line above), whereas `null` means "clear this". So a clear leaves the previous title/artist/album (and progress) on screen — stale now-playing text. The analogous `color@v1` path already handles `None` as a clear via `update_palette(None)`. ## Fix Route `None` through a new `AppState.clear_metadata()`, which drops title/artist/album and the progress fields and reports whether anything changed; a non-null payload keeps the existing merge path unchanged. ## Verification - New `tests/tui/test_metadata_clear.py`: - `clear_metadata()` resets the now-playing fields and is idempotent; - `_handle_metadata_update()` with `metadata=None` clears a populated state. - `pytest` — 149 passed; `ruff check`, `ruff format --check`, `mypy sendspin` clean. ## Types of changes - [x] Bug fix: `bugfix` --- sendspin/tui/app.py | 18 ++++++++++- tests/tui/test_metadata_clear.py | 53 ++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 tests/tui/test_metadata_clear.py diff --git a/sendspin/tui/app.py b/sendspin/tui/app.py index 49573c7..7a3dc32 100644 --- a/sendspin/tui/app.py +++ b/sendspin/tui/app.py @@ -139,6 +139,15 @@ def update_metadata(self, metadata: SessionUpdateMetadata) -> bool: return changed + def clear_metadata(self) -> bool: + """Clear all now-playing metadata and report whether anything changed.""" + changed = False + for attr in ("title", "artist", "album", "track_progress", "track_duration"): + if getattr(self, attr) is not None: + setattr(self, attr, None) + changed = True + return changed + def describe(self) -> str: """Return a human-friendly description of the current state.""" lines: list[str] = [] @@ -811,7 +820,14 @@ def _handle_metadata_update(self, payload: ServerStatePayload) -> None: ui = self._ui if isinstance(payload.metadata, UndefinedField): return - if payload.metadata is None or not state.update_metadata(payload.metadata): + if payload.metadata is None: + # A bare ``metadata: null`` is a spec-legal clear -- an absent field + # is UndefinedField and handled above -- so drop the stale + # now-playing fields instead of leaving the previous track on screen. + changed = state.clear_metadata() + else: + changed = state.update_metadata(payload.metadata) + if not changed: return with ui.batch_update(): diff --git a/tests/tui/test_metadata_clear.py b/tests/tui/test_metadata_clear.py new file mode 100644 index 0000000..becfb25 --- /dev/null +++ b/tests/tui/test_metadata_clear.py @@ -0,0 +1,53 @@ +"""Tests for the TUI metadata-clear handling.""" + +from __future__ import annotations + +import contextlib +from types import SimpleNamespace +from typing import Any + +from sendspin.tui.app import AppState, SendspinApp + + +def _fake_ui() -> SimpleNamespace: + return SimpleNamespace( + batch_update=contextlib.nullcontext, + set_metadata=lambda **kwargs: None, + set_progress=lambda *args: None, + set_repeat_shuffle=lambda *args: None, + add_event=lambda *args: None, + ) + + +def _app_with(state: AppState) -> Any: + return SimpleNamespace(_ui=_fake_ui(), _state=state) + + +def test_app_state_clear_metadata_resets_now_playing() -> None: + state = AppState( + title="Track", + artist="Artist", + album="Album", + track_progress=1000, + track_duration=5000, + ) + + assert state.clear_metadata() is True + assert state.title is None + assert state.artist is None + assert state.album is None + assert state.track_progress is None + assert state.track_duration is None + # Clearing an already-clear state reports no change. + assert state.clear_metadata() is False + + +def test_handle_metadata_update_clears_on_null_payload() -> None: + """A spec-legal metadata clear must not leave stale now-playing text behind.""" + state = AppState(title="Track", artist="Artist", album="Album") + + SendspinApp._handle_metadata_update(_app_with(state), SimpleNamespace(metadata=None)) + + assert state.title is None + assert state.artist is None + assert state.album is None From 74416f1fbafdef4ab383c5f9e4f888cee2066354 Mon Sep 17 00:00:00 2001 From: Paul Robertson Date: Sun, 25 Oct 2026 20:08:09 +0000 Subject: [PATCH 14/15] fix(audio): keep stream open across pause so DACs hold lock Pause arrives as stream/end, which ran close_stream() and fully closed the PortAudio stream; resume then recreated it via set_format(). Each close/open drops USB lock, clicking muting relays on XMOS-based DACs (verified live: the click survives full digital silence, so no server-side mute or fade can suppress it). clear() and close_stream() now drain the queue but leave the stream running - the callback already renders silence while starved - and set_format() skips recreating the stream when the format is unchanged. The device releases on stop() (shutdown) and on real format changes only. --- sendspin/audio.py | 31 +++++++++------- tests/test_audio.py | 88 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 13 deletions(-) diff --git a/sendspin/audio.py b/sendspin/audio.py index 5bf1a63..099dc28 100644 --- a/sendspin/audio.py +++ b/sendspin/audio.py @@ -253,6 +253,14 @@ def set_format(self, audio_format: AudioFormat, device: AudioDevice) -> None: device: Audio device to use. """ pcm_format = audio_format.pcm_format + if self._stream is not None and self._format == pcm_format: + logger.debug( + "Audio format unchanged (%dHz/%d-bit/%dch); keeping stream open", + pcm_format.sample_rate, + pcm_format.bit_depth, + pcm_format.channels, + ) + return self._format = pcm_format self._close_stream() @@ -340,17 +348,16 @@ def stop(self) -> None: self._stream_executor.shutdown(wait=True) def close_stream(self) -> None: - """Drop queued audio and fully close the stream to release the device. + """Drop queued audio but keep the stream open on silence. - Unlike clear(), which only stops the stream (leaving the device FD open), - this fully closes the PortAudio stream. Call when the server signals - end-of-stream; the stream will be recreated by set_format() when the - next track begins. + The callback renders silence while starved, so the audio device + stays open and DACs hold lock across pause/resume and across + stream end/start pairs. The device is released by stop() + (shutdown) and by set_format() when the format actually changes. """ if self._closed: return self.clear() - self._close_stream() def clear(self) -> None: """Drop all queued audio chunks.""" @@ -359,13 +366,11 @@ def clear(self) -> None: # Clear deferred operation flag self._clear_requested = False - # Stop the audio stream (but don't close it) to release ALSA device - # This allows the device to transition to 'closed' state when paused. - # The stream restarts when new chunks arrive in submit(). - self._stream_started = False - stream = self._stream - if stream is not None: - self._stream_executor.submit(self._call_stream, stream.stop) + # Keep the audio stream running: with the queue drained the callback + # renders silence, so the device stays open and DACs that click a + # muting relay on lock loss (e.g. XMOS USB) stay silent across + # pause/resume/seek. Chunks submitted after the clear play on the + # still-running stream; submit() restarts it only if never started. # Drain all queued chunks while True: diff --git a/tests/test_audio.py b/tests/test_audio.py index 3290cb5..2d871a3 100644 --- a/tests/test_audio.py +++ b/tests/test_audio.py @@ -61,3 +61,91 @@ def now() -> int: assert player._drop_every_n_frames == 0 # noqa: SLF001 assert player._insert_every_n_frames == 0 # noqa: SLF001 + + +class _RecordingStream: + def __init__(self) -> None: + self.stops = 0 + + def stop(self) -> None: + self.stops += 1 + + +def test_clear_keeps_stream_open_so_device_never_closes() -> None: + """clear() must drain the queue without stopping the stream. + + The callback renders silence while starved, so keeping the stream + running holds the audio device open and DACs with lock-loss muting + relays (e.g. XMOS USB) stay silent across pause/resume/seek. + """ + player = AudioPlayer(lambda ts: ts, lambda ts: ts, now_us=lambda: 0) + stream = _RecordingStream() + player._stream = stream # noqa: SLF001 + player._stream_started = True # noqa: SLF001 + player._queue.put( # noqa: SLF001 + _QueuedChunk(server_timestamp_us=0, audio_data=b"\x01\x00\x02\x00") + ) + + player.clear() + + assert player._queue.empty() # noqa: SLF001 + assert player._stream_started # noqa: SLF001 + assert stream.stops == 0 + + +class _RecordingStreamFull: + def __init__(self) -> None: + self.stops = 0 + self.starts = 0 + self.closes = 0 + + def stop(self) -> None: + self.stops += 1 + + def start(self) -> None: + self.starts += 1 + + def close(self) -> None: + self.closes += 1 + + +def _make_player() -> AudioPlayer: + return AudioPlayer(lambda ts: ts, lambda ts: ts, now_us=lambda: 0) + + +def test_close_stream_keeps_device_open_on_silence() -> None: + """End-of-stream must not release the audio device. + + The server sends stream/end on pause, so fully closing here is what + drops DAC lock and clicks the relay. Silence keeps it held. + """ + player = _make_player() + stream = _RecordingStreamFull() + player._stream = stream # noqa: SLF001 + player._stream_started = True # noqa: SLF001 + player._queue.put( # noqa: SLF001 + _QueuedChunk(server_timestamp_us=0, audio_data=b"\x01\x00\x02\x00") + ) + + player.close_stream() + + assert player._queue.empty() # noqa: SLF001 + assert player._stream_started # noqa: SLF001 + assert player._stream is stream # noqa: SLF001 + assert (stream.stops, stream.closes) == (0, 0) + + +def test_set_format_same_format_keeps_existing_stream() -> None: + """Resume after pause re-sends the identical format: no recreate.""" + from aiosendspin.client import PCMFormat + + player = _make_player() + stream = _RecordingStreamFull() + fmt = PCMFormat(sample_rate=48000, channels=2, bit_depth=16) + player._format = fmt # noqa: SLF001 + player._stream = stream # noqa: SLF001 + + player.set_format(SimpleNamespace(pcm_format=fmt), device=None) + + assert player._stream is stream # noqa: SLF001 + assert (stream.stops, stream.closes) == (0, 0) From e0e1e9000a7d104bbe95f9f9b08cc8f3da79c690 Mon Sep 17 00:00:00 2001 From: Paul Robertson Date: Sun, 25 Oct 2026 21:33:24 +0000 Subject: [PATCH 15/15] fix(audio): harden callback against stale-format races Resume-after-announce crashed the audio callback: set_format() swapped the shared format synchronously while the old stream closed async, so an in-flight old-stream callback sized its buffer from the new format (ValueError: memoryview assignment), leaving the client silent while the server showed playing. Seen live on a moOde Pi. Three guards: set_format() stops the old stream synchronously before swapping (stop blocks until in-flight callbacks finish; worker thread, never in-callback, and _call_stream logs internally) and drops the stale last-frame cache; the callback clamps bytes_needed and read frames to the actual buffer so any residual mismatch degrades to trailing silence; _fill_silence trims to the buffer. Release paths unchanged (stop(), real format changes). --- sendspin/audio.py | 52 ++++++++++++++++++++++++++++--- tests/test_audio.py | 74 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 4 deletions(-) diff --git a/sendspin/audio.py b/sendspin/audio.py index 099dc28..f54947f 100644 --- a/sendspin/audio.py +++ b/sendspin/audio.py @@ -261,6 +261,18 @@ def set_format(self, audio_format: AudioFormat, device: AudioDevice) -> None: pcm_format.channels, ) return + # Stop the old stream synchronously BEFORE swapping the format. The + # close below is async, and an in-flight old-stream callback computing + # against the new format sizes its buffer wrong and crashes + # (ValueError: memoryview assignment, seen live on resume-after- + # announce). stop() blocks until in-flight callbacks finish, and this + # runs on the worker thread, never in a callback, so no deadlock. + # _call_stream logs internally. Also drop the last-frame cache: it + # carries the old frame size into the slow correction path. + old_stream = self._stream + if old_stream is not None: + self._call_stream(old_stream.stop) + self._last_output_frame = b"" self._format = pcm_format self._close_stream() @@ -433,8 +445,14 @@ def _audio_callback( # noqa: PLR0915 assert self._format is not None - bytes_needed = frames * self._format.frame_size output_buffer = memoryview(outdata).cast("B") + # Trust the buffer, not the parameters: a stale callback racing a + # format switch can arrive with frames sized for another format. + # Every slice below derives from bytes_needed, so bounding it here + # bounds them all (bulk reads are capped separately to keep cursor + # accounting consistent). + got = frames * self._format.frame_size + bytes_needed = got if got < len(output_buffer) else len(output_buffer) if status: # Detect underflow and request re-anchor (processed by main thread) @@ -485,6 +503,22 @@ def _audio_callback( # noqa: PLR0915 else: frame_size = self._format.frame_size + # Hard guarantee: never read more frames than fit the buffer. + # A stale callback racing a format switch would otherwise size + # its read from the new format against the old buffer and crash + # (ValueError: memoryview assignment, seen live). Capped reads + # degrade to a short trailing silence fill instead. + max_frames = ( + (len(output_buffer) - bytes_written) // frame_size if frame_size > 0 else 0 + ) + if frames > max_frames: + logger.debug( + "Capping stale callback read: %d frames to %d", + frames, + max(0, max_frames), + ) + frames = max(0, max_frames) + # Thread-safe snapshot of correction schedule (prevent mid-callback changes) insert_every_n = self._insert_every_n_frames drop_every_n = self._drop_every_n_frames @@ -572,6 +606,12 @@ def _audio_callback( # noqa: PLR0915 self._frames_until_next_insert = insert_counter self._frames_until_next_drop = drop_counter + # Trailing guard: never hand PortAudio uninitialized bytes. A + # capped stale read above can come up short; pad the tail. + if bytes_written < bytes_needed: + self._fill_silence(output_buffer, bytes_written, bytes_needed - bytes_written) + bytes_written = bytes_needed + except Exception: logger.exception("Error in audio callback") # Fill rest with silence on error @@ -912,9 +952,13 @@ def _smooth_sync_error(self, error_us: int) -> None: self._sync_error_filtered_us = self._sync_error_filter.offset def _fill_silence(self, output_buffer: memoryview, offset: int, num_bytes: int) -> None: - """Fill output buffer range with silence.""" - if num_bytes > 0: - output_buffer[offset : offset + num_bytes] = b"\x00" * num_bytes + """Fill output buffer range with silence, trimmed to the buffer.""" + end = offset + num_bytes + buf = len(output_buffer) + if end > buf: + end = buf + if end > offset: + output_buffer[offset:end] = b"\x00" * (end - offset) def _apply_volume(self, output_buffer: memoryview) -> None: """ diff --git a/tests/test_audio.py b/tests/test_audio.py index 2d871a3..f300623 100644 --- a/tests/test_audio.py +++ b/tests/test_audio.py @@ -149,3 +149,77 @@ def test_set_format_same_format_keeps_existing_stream() -> None: assert player._stream is stream # noqa: SLF001 assert (stream.stops, stream.closes) == (0, 0) + + +def test_set_format_change_stops_old_stream_synchronously() -> None: + """A format switch must stop the old stream before swapping formats. + + Otherwise an in-flight old-stream callback computes against the new + format and crashes (ValueError: memoryview assignment, seen live on + resume-after-announce). stop() is safe here: set_format runs on the + worker thread, never inside a callback. + """ + import concurrent.futures + from unittest.mock import patch + + from aiosendspin.client import PCMFormat + + player = _make_player() + old = _RecordingStreamFull() + player._stream = old # noqa: SLF001 + player._format = PCMFormat(sample_rate=48000, channels=2, bit_depth=16) # noqa: SLF001 + player._stream_executor = concurrent.futures.ThreadPoolExecutor(max_workers=1) # noqa: SLF001 + new = _RecordingStreamFull() + new.latency = 0.05 + try: + with patch("sendspin.audio.sounddevice.RawOutputStream", return_value=new): + player.set_format( + SimpleNamespace( + pcm_format=PCMFormat(sample_rate=44100, channels=2, bit_depth=24), + codec=SimpleNamespace(value="flac"), + ), + device=SimpleNamespace(device_id=6), + ) + finally: + # Drain the async close: deterministic counts below. + player._stream_executor.shutdown(wait=True) # noqa: SLF001 + # One synchronous stop (before the swap) + one async stop/close pair. + assert old.stops == 2 + assert old.closes == 1 + assert player._format.sample_rate == 44100 # noqa: SLF001 + assert player._stream is new # noqa: SLF001 + assert player._last_output_frame == b"" # noqa: SLF001 + + +def test_stale_callback_capped_to_buffer_instead_of_crash() -> None: + """Replay the live crash: old-stream buffer, new-format frame size. + + A 2048-frame callback on a buffer sized for 2-byte frames while the + format claims 6-byte frames must degrade to silence, never raise. + """ + now_us = 0 + + def now() -> int: + return now_us + + player = AudioPlayer(lambda ts: ts, lambda ts: ts, now_us=now) + player._format = SimpleNamespace( # noqa: SLF001 + sample_rate=48000, + channels=2, + bit_depth=24, + frame_size=6, + ) + player._playback_state = PlaybackState.PLAYING # noqa: SLF001 + player._queue.put( # noqa: SLF001 + _QueuedChunk(server_timestamp_us=0, audio_data=b"\x01" * 600) + ) + + out = bytearray(2048 * 2) # old stream buffer: 2-byte frames + player._audio_callback( # noqa: SLF001 + memoryview(out), + frames=2048, + time=SimpleNamespace(outputBufferDacTime=0.0), + status=_NoCallbackStatus(), + ) + + assert bytes(out[-2:]) == b"\x00\x00"