Repository navigation
[REVIEW] fix(audio): keep stream open + harden stale-format races - #2
Draft
kiwipaulrob wants to merge 15 commits into
Draft
kiwipaulrob wants to merge 15 commits into
kiwipaulrob wants to merge 15 commits into
Conversation
## 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#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.
…ting 0 (Sendspin#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.
…dspin#288) ## 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.
… the outset (Sendspin#287) ## 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.
## 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.
…spin#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`.
…ALSA devices) (Sendspin#286) ## 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 Sendspin#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 Sendspin#262 (and the same class as Sendspin#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 Sendspin#262 Refs Sendspin#251
# What does this implement/fix? <!-- Quick description and explanation of changes. --> 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: - Sendspin/aiosendspin#450 **Related issue (if applicable):** - <link> ## Types of changes <!-- Tick exactly one box. CI (.github/workflows/pr-labels.yaml) applies the matching label, and the release notes use it to sort this change. --> - [ ] 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`
# What does this implement/fix? <!-- Quick description and explanation of changes. --> 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 <!-- Tick exactly one box. CI (.github/workflows/pr-labels.yaml) applies the matching label, and the release notes use it to sort this change. --> - [ ] 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`
…dspin#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 Sendspin#294 ## Types of changes <!-- Tick exactly one box. CI (.github/workflows/pr-labels.yaml) applies the matching label, and the release notes use it to sort this change. --> - [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`
…endspin#297) # 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`
…spin#289) ## 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`
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.
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review copy - do not merge
Fork-internal review before upstream. Two commits on
fix/keep-stream-open-on-clear:74416f1- keep stream open on clear/close, skip identical-format recreates (DAC lock-hold fix).e0e1e90- harden callback against stale-format races (sync stop before swap, buffer-bounded reads, trailing silence pad, last-frame reset) plus 3 more regression tests.Base
mainis 13 behind upstream; the upstream mirror diff is identical in the two files. Full rationale, evidence and trade-offs: see upstream draft Sendspin#304 (to be refreshed from this review). When approved here, this exact branch goes up as the upstream PR update.