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], + }); + } diff --git a/README.md b/README.md index ecee88f..64869b6 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. @@ -114,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", @@ -147,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 | @@ -185,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/audio.py b/sendspin/audio.py index 5bf1a63..f54947f 100644 --- a/sendspin/audio.py +++ b/sendspin/audio.py @@ -253,6 +253,26 @@ 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 + # 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() @@ -340,17 +360,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 +378,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: @@ -428,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) @@ -480,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 @@ -567,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 @@ -907,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/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/sendspin/cli.py b/sendspin/cli.py index bc06d57..a137797 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 @@ -890,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, ) diff --git a/sendspin/daemon/daemon.py b/sendspin/daemon/daemon.py index 09dffda..640be74 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, @@ -83,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 @@ -355,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: @@ -433,7 +450,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 +474,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: 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) diff --git a/sendspin/settings.py b/sendspin/settings.py index 04a6ca5..582c056 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) @@ -123,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 @@ -222,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") @@ -353,8 +361,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/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/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/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. diff --git a/tests/test_audio.py b/tests/test_audio.py index 3290cb5..f300623 100644 --- a/tests/test_audio.py +++ b/tests/test_audio.py @@ -61,3 +61,165 @@ 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) + + +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" 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" diff --git a/tests/test_settings.py b/tests/test_settings.py new file mode 100644 index 0000000..d18ae77 --- /dev/null +++ b/tests/test_settings.py @@ -0,0 +1,39 @@ +"""Tests for settings persistence.""" + +from __future__ import annotations + +import json +from pathlib import Path + +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: + """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 + + +@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 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 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]