Skip to content

Add a server role: synchronized multi-client audio push, mDNS discovery/advertising, and supervised connections - #87

Draft
davidgraeff wants to merge 12 commits into
Sendspin:mainfrom
davidgraeff:server-role-prototype
Draft

Add a server role: synchronized multi-client audio push, mDNS discovery/advertising, and supervised connections#87
davidgraeff wants to merge 12 commits into
Sendspin:mainfrom
davidgraeff:server-role-prototype

Conversation

@davidgraeff

Copy link
Copy Markdown

Closes #86

What this adds

sendspin-rs currently implements only the client/receiver role. This PR adds a server role (src/server/) so the crate can accept Sendspin clients, sync their clocks, and push synchronized audio out to them — built on the existing protocol::messages schema and binary frame layout, reusing those types.

The concrete driver is a Home Assistant add-on that streams audio to Home Assistant Voice PE, whose ESPHome sendspin: component (sendspin-cpp) only ever runs its own embedded WebSocket server and never dials out. Supporting that device forces both connection directions — inbound accept and discover-then-dial — so the server role handles both.

Everything here has been validated against the real, unmodified aiosendspin[server] reference and tested against real Voice PE hardware on the network.

New public API

pub use server::{ServerConnection, ServerListener};
// and, within the server module:
//   Advertisement, ClientBrowser        // mDNS advertise / discover
//   dial_client                          // one-shot dial-out
//   ClientManager, ClientEvent           // supervised discovery + reconnect
//   Group, DEFAULT_SEND_AHEAD_US         // synchronized multi-client playback
//   ServerConnectionGuard, ServerSender
//   encode_audio_frame                   // encode-side mirror of AudioChunk::from_bytes

How it's built (commit by commit)

The branch is structured so each commit is a reviewable, self-contained step:

  1. feat(server): prototype the server role: ServerListener (accept + server-side handshake, mirroring ProtocolListener for the opposite role), ServerConnection/ServerSender (per-client actor with the same writer-task/message-router split as protocol::client, time-sync echo stamped at actual send time, stream/start → binary chunks → stream/end), and binary::encode_audio_frame (the encode-side mirror of the client's parse-only AudioChunk::from_bytes). Scoped to a single client, single format, player role.

  2. test(conformance): prove the server role matches real aiosendspin: Drives both the real aiosendspin[server] and the new Rust server through an identical canned pcm track, points a real aiosendspin protocol client at each, and diffs what it observed. Audio bytes are sha256-identical and every event (ordering + field values) matches.

  3. feat(server): multi-client synchronized playback groups: Group: the actual multi-room correctness property. Every member is sent the same audio bytes tagged with the same server-clock timestamp for a given push, computed once per push; each client's own clock offset then handles sample-accurate simultaneous playback, so the server never compares members' clocks to each other. Broadcasts concurrently (one slow member can't block the others); a member whose send fails is pruned automatically.

  4. feat(server): mDNS advertising via _sendspin-server._tcp.local.: Advertisement, so clients can discover a running server without a configured address (distinct service type from _sendspin._tcp.local., which is what a client advertises). Promotes mdns-sd from a dev-dependency to a real dependency.

  5. feat(server): discover and dial clients that only run their own server: ClientBrowser (browses _sendspin._tcp.local., resolves each client to a ws:// URL) and dial_client (connects out and drives the same server-role handshake as accepted connections — the protocol roles don't depend on dial direction). This is the path Voice PE requires.

  6. feat(server): ClientManager: continuous discovery + reconnect-with-backoff: Closes the robustness gap: dial_client is one-shot. ClientManager supervises every discovered client — dials on discovery, retries with capped exponential backoff (matching aiosendspin's MAX_RECONNECT_BACKOFF_S/STABLE_SERVER_INITIATED_SESSION_S), re-dials promptly when the same device (tracked by stable mDNS fullname, not ephemeral address) reappears at a new address. Surfaces Connected/Message/Disconnected as one event stream. This is what real deployments should use.

  7. docs(server): close out late-join scope: records the decision that late-join historical replay is not implemented, with a test proving the actual behavior a mid-stream joiner gets.

  8. examples: add play_wav: a manual hardware test tool: streams a WAV file to whatever client(s) connect, running all three connection sources (accept, discover+dial, direct --dial) into one Group.

Scope

This is a v1/prototype scope, not full parity with aiosendspin. Everything deferred is tracked in src/server/mod.rs's module docs.

Deferred (follow-up): per-client codec transcoding/resampling (v1 is PCM-only, one format per group); non-player roles (color, visualizer, artwork, controller, metadata); external player registration.

Not planned, by decision: late-join catch-up / historical buffer replay. aiosendspin's version of this exists almost entirely to re-encode cached history through a newly-joined role's codec/format when it differs from what's cached. A mid-stream joiner gets stream/start plus the next push, in sync with everyone else, but nothing from before it joined; Can result in a short join gap, but everything else would have exploded the code lines of this PR. Current behaviour proven by a_late_joiner_gets_current_stream_start_and_only_subsequent_audio in tests/group_sync.rs.

Testing

  • Conformance (tests/conformance/): observable equivalence to the real aiosendspin[server] — sha256-identical audio, identical event sequence/fields. See tests/conformance/README.md (including a real bug in the oracle driver this caught: registering the event listener after start_server(), which opens the TCP listener before finishing its own mDNS setup).
  • Unit/integration: tests/server_listener.rs, tests/group_sync.rs, tests/discovery.rs (real mDNS browser), tests/dial_discovery.rs (fake self-advertising client), tests/client_manager.rs.
  • Real hardware: tested against Home Assistant Voice PE devices on the network, and end-to-end via examples/play_wav.rs.

Dependency change

mdns-sd moves from a dev-dependency (client examples only) to a real dependency — the server role needs it to advertise and discover.

Notes for reviewers

Happy to adjust module layout, naming, or the public re-export surface. The scope boundaries above are deliberate for a first milestone; I'm open to pulling any of the deferred items forward if you'd prefer them in the initial merge.

…single-client audio push

Sendspin-rs currently only implements the client/receiver role. This adds a
minimal server role (src/server/) built on the existing protocol::messages
schema and binary frame layout, reusing them rather than duplicating types:

- ServerListener: accepts inbound WebSocket connections, drives the
  server-side handshake (client/hello -> server/hello), mirrors
  ProtocolListener's path/TLS handling for the opposite role.
- ServerConnection/ServerSender: per-client actor with the same
  writer-task/message-router split as protocol::client, time-sync echo
  (server/time, stamped at actual send time), and stream/start ->
  binary audio chunks -> stream/end for the player@v1 role.
- binary::encode_audio_frame: the encode-side mirror of
  protocol::client::AudioChunk::from_bytes, which only ever parses (a
  client never sends audio).

Scoped to a single client, single audio format, player role only for this
first milestone — see src/server/mod.rs's doc comment for what's explicitly
deferred (codec transcoding, late-join catch-up, non-player roles, etc.).
… behavior

Adds a conformance suite (tests/conformance/) that drives the real, unmodified
aiosendspin[server] reference implementation and the new Rust server role
through an identical canned stimulus, then points the same real aiosendspin
protocol client at each in turn and diffs what it actually observed:

- oracle_server.py / examples/conformance_server.rs: same fixture audio
  (fixtures/stimulus.pcm), same chunk timing, same volume/mute command
  values at the same points in the stream.
- run_against_client.py: records server/hello, stream/start, audio bytes
  (hashed), server/command, stream/end, disconnect as JSON.
- compare.py: asserts exact audio byte match and identical event
  sequence/field values; deliberately does not assert wire-frame chunk
  count, since aiosendspin's PushStream re-packetizes commits into its own
  granularity — an implementation detail, not a conformance requirement.

Result: audio bytes are sha256-identical and every event matches. Building
this also caught a real bug in the oracle driver itself (registering the
event listener after start_server(), which opens the TCP listener before
finishing its own mDNS setup) — documented in tests/conformance/README.md.
Adds Group (src/server/group.rs) — the actual multi-room correctness
property: every member is sent the *same* audio bytes tagged with the *same*
server-clock timestamp for a given push, computed once per push rather than
per-member. Each client's own independent clock-sync offset (tracked via
client/time <-> server/time, already in place from the previous commit) is
then all that's needed for sample-accurate simultaneous playback — the
server never has to compare members' clocks to each other.

- start_stream/push_audio/send_player_command/end_stream broadcast
  concurrently to every member (one slow member can't block delivery to
  the others).
- A member whose send fails is pruned automatically — the failure means its
  connection's writer task is already gone, so there's nothing to retry.

v1-scoped: one shared PCM format per group, no late-join catch-up, no
historical replay — see src/server/mod.rs's doc comment for the full list.

tests/group_sync.rs proves the actual property with two real WebSocket
peers: identical timestamp and identical bytes delivered to both members
for one push, and that a dead member gets pruned without blocking the
survivor's subsequent command.
Adds Advertisement (src/server/discovery.rs) so clients can discover a
running server without a configured address — matches aiosendspin's
server.py service type (distinct from _sendspin._tcp.local., which is what
a *client* advertises for server-initiated connections, see
examples/server_initiated_metadata.rs). Unregisters and shuts down its
background daemon on drop.

Promotes mdns-sd from a dev-dependency (client examples only) to a real
dependency, now that the server role needs it too.

tests/discovery.rs proves it with a real mDNS browser: resolves the
service and checks port + path/name TXT records match what was advertised.

Retry the mDNS-timing-dependent tests to harden CI against flakiness
A Home Assistant Voice PE only ever runs its own embedded WebSocket server,
it will not dial into an advertised sendspin server (sendspin-cpp).
The server has to discover these clients and dial *them*, mirroring aiosendspin's
SendspinServer.connect_to_client/_start_mdns_discovery.

- ClientBrowser (src/server/discovery.rs): browses `_sendspin._tcp.local.`
  (distinct from `_sendspin-server._tcp.local.`, which is what our own
  Advertisement advertises for the other direction), resolving each
  discovered client to a ws:// URL.
- dial_client (src/server/dial.rs): connects out to that URL and drives the
  exact same server-role handshake ServerConnection::drive already uses for
  accepted connections — the protocol roles don't depend on dial direction.

tests/dial_discovery.rs proves the full path against a fake self-advertising
client.

Has also been tested again real Voice PE devices on the network.
…ackoff

Closes the robustness gap called out in the previous commits: dial_client
was one-shot, with no retry if a device rebooted or dropped WiFi. For a
production add-on (vs. a manual test tool) that's not acceptable — a device
that reboots shouldn't need the add-on restarted to reconnect.

ClientManager supervises every discovered client: dials on discovery,
retries with capped exponential backoff on failure or disconnect (matching
aiosendspin's MAX_RECONNECT_BACKOFF_S/STABLE_SERVER_INITIATED_SESSION_S),
and re-dials promptly if the same device (tracked by mDNS fullname, stable
across address changes) reappears at a new address instead of retrying the
stale one forever. It owns each connection's message loop internally,
surfacing Connected/Message/Disconnected as a single event stream so
callers don't drive raw ServerConnections by hand.
Investigated what aiosendspin's late-join catch-up/historical-buffer-replay
actually does (read push_stream.py's role/PCM chunk caches and catch-up
encoding directly, not from memory). Its complexity almost entirely exists
to re-encode cached history through a newly-joined role's own codec/format
when it differs from what's already cached — moot for us, since v1 has one
shared PCM format per group. Decision: no historical replay. A member that
joins mid-stream gets stream/start plus whatever the next push_audio() call
sends, in sync with the rest of the group, but nothing from before it
joined — a short join gap is acceptable.

That's what Group::add_member already did; this commit is verification, not
new behavior. Added a_late_joiner_gets_current_stream_start_and_only_subsequent_audio
to tests/group_sync.rs — the existing tests only covered members added
*before* start_stream, never the actual "joins after the stream is already
live" case this decision rests on. Updated src/server/mod.rs's scope notes
to record the decision and why, instead of listing this as still-deferred.
play_wav streams a WAV file (plain PCM) to whatever
Sendspin client(s) connect: waits for the first connection, gives a short
grace window for additional clients to join as one synchronized Group (for
testing multi-room sync against real hardware), streams the file, and
prints every client/state, client/command, and client/goodbye it receives
along the way.

play_wav runs three connection sources, feeding whichever finds clients into the same Group:
* inbound accept,
* mDNS discovery + auto-dial of self-advertising clients (--discover, on by default),
* and directly dialing a known address (--dial ws://host:port/path, repeatable).

Uses ClientManager: automatic reconnect-with-backoff and address-change handling
apply to the discovery path.

Smoke-tested end to end against the real aiosendspin protocol client
(tests/conformance/run_against_client.py standing in for real hardware):
correct format negotiation, all audio bytes delivered, clean disconnect
handling all confirmed working before pointing this at anything real.
@davidgraeff
davidgraeff force-pushed the server-role-prototype branch from 1594c06 to b85b659 Compare July 14, 2026 10:31
@davidgraeff

Copy link
Copy Markdown
Author

Because there was no review yet, I have for pushed two small fix (use SO_REUSEADDR for server, ClientManager: Propagate fullname) into the existing commits.

DanielHabenicht added a commit to DanielHabenicht/fork.sendspin-rs that referenced this pull request Jul 14, 2026
…ndspin#70)

Implements the optional `mac_address` field on `device_info` in
`client/hello` as merged in
[Sendspin/spec#87](Sendspin/spec#87).

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
DanielHabenicht pushed a commit to DanielHabenicht/fork.sendspin-rs that referenced this pull request Jul 14, 2026
## 🤖 New release

* `sendspin`: 0.2.1 -> 0.3.0 (⚠ API breaking changes)

• The unused `AudioOutput` trait/output code is removed; cpal is used directly. Sendspin#46 (breaking)
• The internal custom `Sample` type is replaced with native `i32` and cpal's `Sample` methods. Sendspin#47 (breaking)
• New `request_format`, `request_artwork_format`, and `stream` message emission helpers. Sendspin#56 / Sendspin#63
• `ProtocolListener` (`listen`/`accept`) lets the SDK accept server-initiated peers, with optional TLS and HTTP-path routing. Sendspin#57 (breaking)
• `ConnectionGuard::disconnect` now owns its sender and drops the `&WsSender` argument. Sendspin#57 (breaking)
• The writer is channel-driven, so `send_message`/`disconnect` now surface real socket failures. Sendspin#57
• The SDK can declare the metadata role. Sendspin#58
• A new `server_initiated_metadata` example demonstrates the listener, metadata role, mDNS, and multi-server arbitration. Sendspin#60
• Output buffer size is now configurable via `SyncedPlayer::new` / `with_process_callback`. Sendspin#61
• `SyncedPlayer` supports more hardware sample formats. Sendspin#62
• `static_delay_ms` is applied in the `SyncedPlayer` playback path. Sendspin#65
• Kalman drift is gated behind an SNR check, matching sendspin-cpp. Sendspin#64
• `repeat`/`shuffle` move to `ControllerState` and are deprecated on `MetadataState`. Sendspin#66 / Sendspin#73
• `AudioScheduler` and `AudioBuffer::play_at` are removed; `SyncedPlayer` handles time conversion live. Sendspin#67 (breaking)
• A new external source API is added. Sendspin#68
• Playback underrun recovery buffering keeps synced playback from stalling. Sendspin#69 (breaking)
• `DeviceInfo` gains a `mac_address` field per spec PR Sendspin#70
• Synced playback startup handoff is fixed. Sendspin#71
• Sync state is aligned to the sendspin spec. Sendspin#72 (breaking)

### ⚠ `sendspin` breaking changes

```text
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field DeviceInfo.mac_address in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/messages.rs:120
  field PlayerState.required_lead_time_ms in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/messages.rs:274
  field PlayerState.min_buffer_ms in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/messages.rs:277
  field ControllerState.repeat in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/messages.rs:396
  field ControllerState.shuffle in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/messages.rs:399
  field Connection.server_hello in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/client.rs:110
  field Connection.server_hello in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/client.rs:110
  field Connection.server_hello in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/client.rs:110

--- failure copy_impl_added: type now implements Copy ---

Description:
A public type now implements Copy, causing non-move closures to capture it by reference instead of moving it.
        ref: rust-lang/rust#100905
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/copy_impl_added.ron

Failed in:
  sendspin::protocol::messages::ClientSyncState in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/messages.rs:294

--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---

Description:
The enum's variant had its discriminant value change. This breaks downstream code that used its value via a numeric cast like `as isize`.
        ref: https://doc.rust-lang.org/reference/items/enumerations.html#assigning-discriminant-values
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/enum_no_repr_variant_discriminant_changed.ron

Failed in:
  variant ClientSyncState::ExternalSource 2 -> 1 in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/messages.rs:299

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/enum_variant_missing.ron

Failed in:
  variant ClientSyncState::Error, previously in file /tmp/.tmpwzm48r/sendspin/src/protocol/messages.rs:289

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters, not counting the receiver (self) parameter.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/method_parameter_count_changed.ron

Failed in:
  sendspin::protocol::client::ConnectionGuard::disconnect takes 2 parameters in /tmp/.tmpwzm48r/sendspin/src/protocol/client.rs:401, but now takes 1 parameters in /tmp/.tmp3CZmcy/sendspin-rs/src/protocol/client.rs:623
  sendspin::audio::synced_player::SyncedPlayer::new takes 5 parameters in /tmp/.tmpwzm48r/sendspin/src/audio/synced_player.rs:229, but now takes 3 parameters in /tmp/.tmp3CZmcy/sendspin-rs/src/audio/synced_player.rs:301
  sendspin::audio::synced_player::SyncedPlayer::with_process_callback takes 6 parameters in /tmp/.tmpwzm48r/sendspin/src/audio/synced_player.rs:268, but now takes 4 parameters in /tmp/.tmp3CZmcy/sendspin-rs/src/audio/synced_player.rs:339
  sendspin::audio::SyncedPlayer::new takes 5 parameters in /tmp/.tmpwzm48r/sendspin/src/audio/synced_player.rs:229, but now takes 3 parameters in /tmp/.tmp3CZmcy/sendspin-rs/src/audio/synced_player.rs:301
  sendspin::audio::SyncedPlayer::with_process_callback takes 6 parameters in /tmp/.tmpwzm48r/sendspin/src/audio/synced_player.rs:268, but now takes 4 parameters in /tmp/.tmp3CZmcy/sendspin-rs/src/audio/synced_player.rs:339

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/module_missing.ron

Failed in:
  mod sendspin::scheduler::audio_scheduler, previously in file /tmp/.tmpwzm48r/sendspin/src/scheduler/audio_scheduler.rs:4
  mod sendspin::audio::output::cpal_output, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/output/cpal_output.rs:4
  mod sendspin::audio::output, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/output/mod.rs:4
  mod sendspin::scheduler, previously in file /tmp/.tmpwzm48r/sendspin/src/scheduler/mod.rs:4

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/struct_missing.ron

Failed in:
  struct sendspin::audio::output::cpal_output::CpalOutput, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/output/cpal_output.rs:14
  struct sendspin::audio::output::CpalOutput, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/output/cpal_output.rs:14
  struct sendspin::audio::CpalOutput, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/output/cpal_output.rs:14
  struct sendspin::scheduler::audio_scheduler::AudioScheduler, previously in file /tmp/.tmpwzm48r/sendspin/src/scheduler/audio_scheduler.rs:10
  struct sendspin::scheduler::AudioScheduler, previously in file /tmp/.tmpwzm48r/sendspin/src/scheduler/audio_scheduler.rs:10
  struct sendspin::AudioScheduler, previously in file /tmp/.tmpwzm48r/sendspin/src/scheduler/audio_scheduler.rs:10
  struct sendspin::audio::types::Sample, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/types.rs:11
  struct sendspin::audio::Sample, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/types.rs:11

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field play_at of struct AudioBuffer, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/types.rs:134
  field play_at of struct AudioBuffer, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/types.rs:134

--- failure trait_missing: pub trait removed or renamed ---

Description:
A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/trait_missing.ron

Failed in:
  trait sendspin::audio::output::AudioOutput, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/output/mod.rs:14
  trait sendspin::audio::AudioOutput, previously in file /tmp/.tmpwzm48r/sendspin/src/audio/output/mod.rs:14
```

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[0.3.0](Sendspin/sendspin-rs@v0.2.1...v0.3.0)
- 2026-06-30

### Added

- [**breaking**] Align to sendspin spec for sync state
([Sendspin#72](Sendspin#72))
- add mac_address field to DeviceInfo per spec PR Sendspin#87
([Sendspin#70](Sendspin#70))
- support more hardware formats
([Sendspin#62](Sendspin#62))
- [**breaking**] Add playback underrun recovery buffering
([Sendspin#69](Sendspin#69))
- Add external source API
([Sendspin#68](Sendspin#68))
- emit stream/request-format messages
([Sendspin#56](Sendspin#56))
([Sendspin#63](Sendspin#63))
- apply static_delay_ms in the SyncedPlayer playback path
([Sendspin#65](Sendspin#65))
- gate Kalman drift behind SNR check before conversion
([Sendspin#64](Sendspin#64))
- [**breaking**] add option to overwrite the default buffer size
([Sendspin#48](Sendspin#48))
- [**breaking**] add inbound WebSocket listener for server-initiated
connections ([Sendspin#57](Sendspin#57))
- add metadata role support
([Sendspin#58](Sendspin#58))

### Fixed

- [**breaking**] use cpal sample instead of custom implementation
([Sendspin#47](Sendspin#47))
- pr#48 left synced_player uncompilable
([Sendspin#61](Sendspin#61))
- [**breaking**] remove unused output code
([Sendspin#46](Sendspin#46))

### Other

- Add repeat/shuffle to ControllerState; deprecate on MetadataState
([Sendspin#66](Sendspin#66))
([Sendspin#73](Sendspin#73))
- Fix synced playback startup handoff
([Sendspin#71](Sendspin#71))
- [**breaking**] remove vestigial AudioScheduler and
AudioBuffer::play_at
([Sendspin#67](Sendspin#67))
- add server-initiated metadata example
([Sendspin#60](Sendspin#60))

### Removed

- [**breaking**] Remove unused `AudioScheduler` and the
`AudioBuffer::play_at` field — superseded by `SyncedPlayer`, which
converts server timestamps to local play time live in the output
callback rather than baking in a schedule that goes stale when the clock
estimate moves
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@teancom

teancom commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@davidgraeff Hello! Thank you so much for taking a crack at this, it's been on my todo list for a long time and I hadn't gotten around to it. Please understand that the following feedback is with the intent of landing the PR, not blowing you off. It took me a few days to respond to the PR because I wanted to give you real, actionable feedback.

Let's start with: please take out all of the conformance adapter stuff. We have a separate conformance repo - github.com/Sendspin/conformance/ - that has a fleet of scenarios that, while not comprehensive (yet!) ensure interoperability between implementations, and is the proper place for that. You can run it locally, adding sendspin-rs as a server, and test this against all of the clients, not just ourselves + aiosendspin. I've found it invaluable for finding bugs. NB: head of aiosendspin as of this writing is in a bit of a broken state when acting as a server as they're adding encryption support, which was recently added to the spec. That should be ignorable for your purposes, just letting you know.

Next, Group should anchor stream_position_us = now + lead on first push (or start_stream), advance it per push by samples * 1_000_000 / rate with a residue accumulator (it already knows the format from StreamPlayerConfig), rebase forward only when the timeline falls behind now + min_lead, and re-anchor on stream/clear. That makes push_audio pacing-insensitive, deletes the need for play_wav's interval-tick workaround, and matches the oracle it's claiming equivalence with. This is the biggest issue by far, as it's the cause of all of the bad audio you were fighting against.

I was looking at play_wav, and was going to mention that you had a problem where you're holding onto the group mutex the whole time preventing add_member during playback and you should switch to a lock per push, but that made me look deeper. And having Group conflate the control plane and the data plane behind one &mut self is the underlying issue. Even with lock-per-push, push_audio doesn't return until every member's actual socket write completes — ServerSender::send_audio_chunk awaits an ack that the writer task fires only after sink.send().await finishes, and broadcast_and_prune joins all members, so each push blocks for the slowest member with no timeout anywhere. A member that errors gets pruned; a member that stalls (TCP zero-window, half-dead Wi-Fi device) blocks push_audio indefinitely. Combine with the mutex and one sick client freezes the entire group: audio to everyone else, membership, everything. So even the "correct" usage pattern has an unbounded starvation hazard baked into the API.

But! Each connection already has a per-client unbounded channel + dedicated writer task — the exact machinery for non-blocking fan-out. push_audio blocks only because it on awaits per-write acks (which exist to detect dead members for pruning). If broadcast just enqueued into each member's channel and pruning happened on writer-task death instead of per-send error, push_audio would be near-instant, a slow member couldn't block the group (its frames queue; its problem), and the whole mutex question would become academic.

So, either give Group interior mutability (&self methods, members behind an internal lock) with non-blocking enqueue broadcast, or restructure it as an actor task that owns membership and takes commands over a channel (same thing, different syntax). Either approach will need some sort of eviction policy so we don't have unbounded memory growth: queue depth, age watermark, something like that. Then play_wav can demonstrate the canonical API usage, including mid-playback joins (yay).

ClientManager address-change swallows Disconnected. When the browser sees the same fullname at a new URL, it abort()s the live supervisor task. A currently-connected client dies without a Disconnected event, so caller bookkeeping desyncs until a send fails. A possible fix would be to stop using abort() as the signaling mechanism: give each supervisor a tokio::sync::watch::Receiver<String> (or a cancellation token + new URL), select! it against the drain, and on change close the connection, emit Disconnected, and redial at the new URL.

There are some references in code comments to your local agent plan docs (spikes/03-sendspin-pushstream). In fact, there are various comments that seem to narrate what happened during a coding session - stuff like:

// ABOUTME: Not a test binary itself — `tests/common/` (mod.rs, not
// ABOUTME: common.rs) is the standard way to share code between
// ABOUTME: integration test files without cargo compiling it as its own test.

or

//   - Clients that only run their own embedded WebSocket server and never
//     dial out — this is the primary topology for real embedded hardware
//     (e.g. ESPHome's `sendspin:` component / Home Assistant Voice PE, via
//     sendspin-cpp), confirmed by reading esphome/esphome's sendspin_hub.cpp:
//     its setup() only ever calls start_server(), never an outbound connect.

that don't belong (amongst other examples). That should all be cleaned up. Oh, and src/server/listener.rs:30 references examples/minimal_server.rs, which doesn't exist.

connection_reason is hardcoded to Discovery even on the dial path.

Discovery is IPv4-only, we should accept IPv6 as well (aiosendspin is biased towards IPv4 but does work with IPv6)

resolve_client_url filters link-local/unspecified but not loopback — sorted-first selection would prefer 127.x if ever advertised.

push_audio clones the payload once per member; an Arc<[u8]>/Bytes would drop that to one allocation.

No mDNS ServiceRemoved handling — a permanently-gone device is redialed forever at the 300s cap. You implemented most of the aiosendspin approach, but only for persistent clients that have retry_indefinitely=True set.

Speaking of, your mDNS tests do real multicast which is fine for now, but should be gated behind something like an env var (SENDSPIN_NET_TESTS=1 or whatever).

Feel free to ask questions here or in the discord and push back if I got something wrong, it's just my brain + Fable over here, so mistakes are more than possible, they're likely. 😄

- Trim verbose ABOUTME/doc headers and remove coding-session narration
  (cross-repo investigation notes, tutorial asides) from the server modules,
  the play_wav example, and the tests; keep concise doc comments.
- Fix a stale doc reference to a nonexistent examples/minimal_server.rs.
- Gate the real-multicast integration tests (discovery, dial_discovery,
  client_manager) behind the SENDSPIN_NET_TESTS env var so a plain
  `cargo test` no longer depends on mDNS multicast being available.
Interoperability is covered by the dedicated conformance suite at
github.com/Sendspin/conformance, which exercises implementations against a
fleet of client scenarios rather than a single canned stimulus. Drop the
local Python adapter (tests/conformance/) and its example driver
(examples/conformance_server.rs), and the now-unused .gitignore entries.
- ClientManager no longer aborts a live supervisor task on an address change,
  which dropped the connection without emitting Disconnected and desynced
  callers. Each supervisor now takes a watch-channel directive: an address
  change redirects it (close connection, emit Disconnected, redial the new
  URL immediately) and an mDNS ServiceRemoved stops it gracefully instead of
  redialing a gone device forever.
- ClientBrowser surfaces removals: next_event() yields Found/Removed
  (Discovered), with next_client()/next_client_url() kept as resolved-only
  conveniences.
- resolve_client_url accepts IPv6 (bracketed in the URL) as well as IPv4, and
  filters loopback in addition to link-local/unspecified so a stable, routable
  address is chosen.
- The dial path now announces connection_reason = Playback (it dialed out to
  stream); inbound accepts remain Discovery. Previously both were hardcoded to
  Discovery.
@davidgraeff

davidgraeff commented Jul 18, 2026

Copy link
Copy Markdown
Author

Hi @teancom,

Thanks for the detailed review! It’s usually not my style to surprise an open-source project with a 3k-line PR. I did consider splitting it up beforehand, but couldn't quite find a clean or practical way to do so.

While play_wav is my primary test application, I admittedly didn't account for edge cases like stalls caused by poor connections. In my small, stable local network with only three sendspin clients, that issue simply never came up.

Thank you also for pointing out the Sendspin/conformance suite, I clearly didn't search thoroughly enough for that.

I will push all upcoming changes as separate commits (no force-pushes) in case you want to track the updates commit by commit.

…o timeline

The old Group conflated control and data behind `&mut self` and blocked
push_audio on every member's socket write (ServerSender::send_audio_chunk
awaited a per-write ack, and the broadcast joined all members), so one stalled
member — TCP zero-window, half-dead Wi-Fi — froze audio, membership, and every
other member. It also recomputed each chunk's timestamp as `now + lead`, making
playback sensitive to push cadence (the root cause of the audio glitches).

Data plane:
- ServerConnection gains a non-blocking `enqueue_audio` that hands the frame to
  the connection's existing writer task and returns immediately (no ack). A
  per-connection backlog counter bounds queued audio (MAX_QUEUED_AUDIO_FRAMES);
  a slow member's frames are evicted rather than growing memory, and a member
  whose writer task has died is reported so the group can prune it.
- Group takes `&self` with its state behind an internal lock, so a broadcast
  never blocks membership changes or other members. It encodes each chunk once
  into a `Bytes` and fans it out as cheap refcount clones (one allocation, not
  one per member). push_audio is now synchronous and near-instant.

Timeline:
- Group anchors `next_ts_us` at `now + send_ahead_us` on the first push after a
  start/clear and advances it by each chunk's exact duration (with a
  fractional-microsecond residue accumulator so it doesn't drift), re-anchoring
  forward only if pushes fall behind. Timestamps no longer depend on push
  cadence.

play_wav now uses the Group directly (no external Mutex, no absolute-schedule
ticker workaround) and demonstrates mid-playback joins: it holds no lock while
streaming, so clients discovered mid-stream join the group in progress.
@davidgraeff

Copy link
Copy Markdown
Author

Thank you again for the feedback, none of it read as blowing me off.
I think I have addressed all of your very valid comments;
details below, point by point in your order.

Conformance adapter — removed. Dropped tests/conformance/ and the
conformance_server.rs example entirely. Instead of a bespoke oracle I wrote a
sendspin-rs server adapter for your conformance repo and opened it as a draft:
Sendspin/conformance#98. It drives
this server role through client-initiated-pcm, server-initiated-pcm, and
server-initiated-pcm-24bit, all passing against the sendspin-rs client
adapter (canonical PCM hashes match). It's draft because it depends on this PR's
server role and because aiosendspin HEAD as a client trips the client/init
handshake (see the last note below).

Group timeline anchoring — done. This was the big one and you were right
about the cause. Group now anchors next_ts_us = now + send_ahead_us on the
first push after a start/clear, advances it by each chunk's exact duration with
a fractional-microsecond residue accumulator (it takes the format from
StreamPlayerConfig), re-anchors forward only when the timeline falls behind
now + send_ahead_us/2, and resets on start_stream/end_stream. push_audio
is pacing-insensitive now, and play_wav's absolute-schedule ticker workaround
is gone.

Group control/data-plane split + non-blocking broadcast — done. You
correctly identified the starvation hazard baked into the API. Group now takes
&self with its members behind an internal lock, and broadcast is non-blocking:
ServerConnection gained enqueue_audio, which hands the frame to the existing
per-connection writer task and returns immediately — no per-write ack. Pruning
now happens on writer-task death (the send fails) rather than per-send error, so
a stalled member can't block the group. I went with interior mutability rather
than a full actor, as the smaller change that reuses the existing writer/channel
machinery.

Eviction: each connection has a bounded audio backlog
(MAX_QUEUED_AUDIO_FRAMES, currently 32 frames); once it's full, further frames
for that member are dropped rather than growing memory without bound — its audio
suffers, nobody else's does. Happy to make that configurable or tune the number;
Maybe you have a smarter idea.

push_audio per-member clone — fixed. Each chunk is encoded once into a
Bytes and fanned out to members as refcount clones — one allocation per push
instead of one per member.

play_wav — reworked. It uses the Group directly now (no external
Mutex, no ticker), holds no lock while streaming, and so demonstrates
mid-playback joins: clients discovered mid-stream are added concurrently by the
accept/discovery loops and join the group in progress.

ClientManager swallowing Disconnected on address change — fixed.
Replaced the abort() signaling with a watch directive channel per
supervisor. An address change now sends Dial(new_url), which makes the
supervisor drop the current connection, emit Disconnected, and redial the
new URL; select! runs the directive against the drain, exactly as you
suggested.

Agent-plan narration / stray comments — cleaned up. Removed the
coding-session narration (the ESPHome/aiosendspin travelogues, the tutorial
asides, the spikes/… references) and tightened the ABOUTME headers to match
the terse two-line style already used across the crate. Also fixed
src/server/listener.rs's reference to the nonexistent
examples/minimal_server.rs.

connection_reason hardcoded to Discovery on the dial path — fixed. The
dial path now sends Playback. I left the inbound accept path as Discovery
(the client initiated, so the server is just present/available) — happy to make
both Playback if you'd prefer.

Discovery IPv4-only — fixed. resolve_client_url now uses
get_addresses() and handles IPv6 (bracketed in the URL) alongside IPv4.

resolve_client_url loopback — fixed. It now filters loopback in addition
to link-local/unspecified, so 127.x/::1 can't be picked as the sorted-first
address.

No ServiceRemoved handling — fixed. ClientBrowser now surfaces
Removed alongside Found (a Discovered enum via next_event()), and
ClientManager stops supervising a device whose mDNS advertisement is removed
(gracefully, emitting Disconnected for a live connection) instead of redialing
a gone device forever.

mDNS tests doing real multicast — gated. The multicast integration tests
(discovery, dial_discovery, client_manager) are now behind
SENDSPIN_NET_TESTS; a plain cargo test no longer depends on multicast.


When using the conformance suite, I noticed that aiosendspin now
opens with client/init (the encryption negotiation you mentioned is in flight?).
The v1 server expects client/hello first.

Should the server skip unknown pre-hello messages for forward-compatibility, or
is that better handled once the encryption handshake is specced? I didn't change
it here.

cargo test / cargo fmt --check are green (17/17 test binaries;
tests/group_sync.rs covers the reworked Group; identical timestamped audio to
all members, a dead member pruned without blocking the survivor, and correct
late-join). Pre-existing chunks_exact clippy lints in src/audio/decode/pcm.rs
are left untouched as out of scope.

@teancom

teancom commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Thanks for the follow-up! Two findings this go 'round:

Control and close messages can still be starved by a stalled audio writer

enqueue_audio() is non-blocking from the group’s perspective, but audio, control, and close commands still share the same per-connection writer queue.

The writer handles an audio command with:
sink.send(WsMessage::Binary(frame)).await

If that socket stalls, the writer cannot process the following commands until the send completes. Consequently, a later stream/end, stream/clear, player command, or Close command can remain queued indefinitely. ServerConnectionGuard::disconnect() also waits for the close acknowledgement, so it can hang in this situation.

This is slightly different from the original group-level blocking problem: the group can enqueue audio without waiting, but lifecycle and cleanup operations for a stalled member can still be blocked behind that audio.

It would probably be useful to add a regression test with a deliberately stalled socket and a subsequent control or close command.

Stream lifecycle transitions are not serialized with audio enqueueing

start_stream() updates stream_config and releases the group lock before it broadcasts stream/start. A concurrent push_audio() can therefore enqueue an audio frame before stream/start.

The inverse happens in end_stream(): it clears the stream state and releases the lock before broadcasting stream/end. A concurrent push_audio() can enqueue another audio frame while the end transition is still in progress.

Because these operations ultimately use the same per-connection writer queue, the client can observe invalid ordering such as:
audio
stream/start

or:
stream/end
audio

The existing tests cover the sequential behavior, but not these concurrent interleavings. The lifecycle transition and audio enqueue path need some sort of shared ordering mechanism so that stream/start, audio, and stream/end are serialized consistently. A concurrent start/push/end test would help demonstrate the guarantee.

davidgraeff pushed a commit to davidgraeff/sendspin-rs that referenced this pull request Jul 28, 2026
…ustainable

Nine changes, all breaking, all cheaper now than after Sendspin#87 merges. No behaviour
changes.

**One `ServerRole` replaces eight entry points.** There were three `dial_client*`
free functions and four `ClientManager::start*` constructors, each adding one
optional parameter to the last — and the telescope had already failed: because
`dial_client_with_reason` hardcoded the write timeout and
`dial_client_with_write_timeout` hardcoded the reason, three functions covered
three of four combinations, and the missing one was real — a *supervised*
connection could not be given a non-default write timeout even though the accept
path had a setter for it. Two of the eight had no callers at all, and the widest
took six positional parameters. Identity and per-connection settings now live in
one value with `dial`, `bind` and `manage` on it, mirroring the client role's
`ProtocolClientBuilder`. It also fixes a hazard of its own: `server_id` and
`server_name` were adjacent strings whose transposition is silent and, since the
client persists `server_id` to recognise this server, lasting — now they are named
once. `spawn_supervisor`/`supervise` take the role too, which is what dropped them
back under clippy's argument limit rather than being annotated past it.

**`AudioFrame` instead of a raw `Bytes`.** `queue_audio` took an already-framed
`tokio_tungstenite::tungstenite::Bytes` — a third-party type in a public parameter
with no re-export, so calling it meant depending on a version-locked
`tokio-tungstenite` — while its neighbour `send_audio_chunk` takes an *unframed*
payload. Passing raw PCM to the queueing path compiled and emitted a silently
malformed frame. "Framed" is now a fact about the type.

**Three names that meant the opposite of what they said.** `push_encoded` does the
encoding (the *timestamp* is what is pre-supplied) → `push_at`. `AudioEnqueue::Sent`
meant queued, not sent → `Queued`; `Evicted` implies an older frame was discarded
when it is the new one → `Dropped`. `enqueue_audio` broke the file's own rule that
`queue_` is synchronous and `send_` awaits → `queue_audio`.

**`AudioEnqueue` is a plain enum, not `Result<AudioEnqueue, Error>`.** The `Err`
arm had one producer, no detail, allocated a `String` on the audio hot path, and
was discarded by every caller — `?` could never help, because a dead member is not
the pusher's failure. Three variants, `Disconnected` among them.

**`QueuedControl` implements `IntoFuture`**, so `sender.queue_player_command(cmd)
.await?` works directly and the four `send_*` methods — 45 lines that were
literally `self.queue_x(..).written().await` — are gone. The queue-then-await-later
shape `Group` needs is unaffected.

**`Group::broadcast`** holds the queue-under-the-lock, settle-outside-it discipline
that is the whole correctness argument of the file, instead of it being copy-pasted
into four method bodies.

**`connection.rs` splits on the writer seam** (987 → 669 lines, `writer.rs` 339).
The writer is fed only through channels, so the interface is four `pub(super)`
items; this also gives the ordering policy a home where it can be unit-tested
against a mock sink rather than only through a real socket.

**`ClientManager::stop_client`** — the counterpart to refusing to give up on a
device whose mDNS record merely lapsed. Since a missed announcement is not evidence
a device left, deciding it *has* is the caller's call, and there was no way to say
so: an embedder had to drop and rebuild the whole manager, every other device's
connection included. `Directive::Stop` is constructed again, so its
`#[allow(dead_code)]` and the doc explaining why nothing used it are gone. Adds
`supervised()` to see what is being managed.

**Deletions and doc corrections.** `ClientBrowser`'s two layered URL accessors
(they discarded the `fullname` that is the stable device identity, i.e. existed to
make the wrong choice convenient); the duplicate `DEFAULT_SEND_AHEAD_US`
re-export, whose "source compatibility" justification was false for a role that
has never shipped; the seven identical `Error::WebSocket("connection closed")`
constructions, now one `connection_closed()`. `mdns_sd` is re-exported whole rather
than as two cherry-picked types, so a consumer's version unifies by construction,
with a note that it is therefore part of this crate's public API.
`ServerConnection::split` makes the exported `ServerConnectionGuard` reachable —
until now no caller could obtain one. Stale docs fixed: `ClientManager` still
claimed it stops supervising on mDNS removal (reversed two commits ago, and
contradicted by its own inline comment 140 lines away), `ClientEvent::Disconnected`
said a reconnect runs only "if it's still discoverable", and `drive` claimed to be
shared with tests it is not reachable from. `cargo doc` is now warning-free.

`tests/common/` grows from retry helpers into the server-role fixtures — bare
peers, stream configs, frame draining, listener binding — removing ~190 lines of
verbatim duplication across six test files (`test_hello` was copied five times,
`connect_peer` twice, `pcm_config` seven). It also fixes a `///` block bug there
that left `retry_flaky` undocumented.
@teancom
teancom marked this pull request as draft August 13, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a server role to sendspin-rs

3 participants