From 4dc6799be9fe4cf6d33750a68c8a3f42e6b099c2 Mon Sep 17 00:00:00 2001 From: Chris Uthe Date: Tue, 15 Sep 2026 19:59:23 -0500 Subject: [PATCH 1/3] Re-pin sendspin-cpp to v0.8.0 and adopt start()/stop() start_server() is deprecated at v0.8.0, and CI builds with -Werror, so the rename is forced rather than optional. stop() replaces the disconnect() plus hand-rolled drain loop: it goodbyes every peer and delivers on_stream_end() before returning, so the stop hook is queued by the time control comes back and hooks.flush() runs it. That makes SHUTDOWN_DRAIN_MS and its timeout warning dead. liveness_timeout_ms is left unset, taking the library's derived ~60 s default. MAX_RETRY_DELAY_MS re-checked against NURSERY_ESTABLISH_TIMEOUT_S, still 30 s. --- CMakeLists.txt | 2 +- docs/ROADMAP.md | 40 +++++++++++++++++++--------------------- src/audio_sink.h | 4 ++-- src/main.cpp | 27 ++++++--------------------- 4 files changed, 28 insertions(+), 45 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c0b729c..eb45561 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -25,7 +25,7 @@ endif() # sendspin-cpp -set(SENDSPIN_GIT_TAG "v0.7.2" CACHE STRING +set(SENDSPIN_GIT_TAG "v0.8.0" CACHE STRING "sendspin-cpp git tag to build against") # player, metadata and controller (group transport verbs); no FORCE, so -D overrides still win. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 2a54ac6..d953e9a 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -444,7 +444,7 @@ the five files that log, and `tests/daemon_test.cpp`: process acquires *any* resource — rather than as a list of three current call sites. All three are live: `make_audio_sink()` already probes the device (`src/audio_sink.cpp:258,267`) and `PortAudioSink` then holds a `PortAudioGuard` that - brings up the CoreAudio HAL's mach ports and helper threads, `start_server()` starts the + brings up the CoreAudio HAL's mach ports and helper threads, `start()` starts the sync task's `std::thread`, and a `DNSServiceRef` is a per-process connection to `mDNSResponder`/`avahi-daemon`. Only the forking thread survives a fork, so an item 7 control socket or an item 8 config file cannot quietly land above the line without @@ -494,7 +494,7 @@ the five files that log, and `tests/daemon_test.cpp`: mDNS daemon. **Three asks removed from this item rather than deferred**, all for one reason worth -writing down: sendspin-cpp v0.7.0 logs through `src/platform/logging.h`'s bare +writing down: sendspin-cpp v0.8.0 logs through `src/platform/logging.h`'s bare `fprintf(stderr, "E %s: " fmt "\n", tag, ...)` macros, gated on a single global `int ss_host_log_level`, with **no callback or sink hook**. Library lines cannot be reformatted, redirected or filtered per tag by any API call this layer can make. @@ -614,7 +614,7 @@ stops the worse failure this item only documents: an operator running a player t authenticated. It is a breaking change for anyone passing such a URL today, which is why it is owed here rather than done here. Masking is what this item ships; refusing is the fix. -**A third thing this item does not claim: the library's own dial lines.** sendspin-cpp v0.7.0 +**A third thing this item does not claim: the library's own dial lines.** sendspin-cpp v0.8.0 logs the URL it is dialling at `info` from `ConnectionManager::connect_to()` and again at `error` from `SendspinClientConnection`, through the same sink-less `SS_LOG*` macros that put timestamps and per-tag filtering out of reach above — so those lines still carry whatever the @@ -942,9 +942,9 @@ advertised an adjustable delay with nothing behind it, leaving a spec requiremen `ConnectionManager::fnv1_hash()`, which lives in the library's uninstalled `src/` — so we store the number we are handed and hand it back, and never compute it. Discovery's own tie-break needs the raw id, which the hash cannot be turned back into. -- **`CliPersistenceProvider` is installed before `add_player()` and `start_server()`**, and +- **`CliPersistenceProvider` is installed before `add_player()` and `start()`**, and neither is negotiable: the pointer is copied into `PlayerRole` at construction, and - `start_server()` is what loads the remembered hash. Installed after either, it is a provider + `start()` is what loads the remembered hash. Installed after either, it is a provider the library never asks. - **Volume and mute are the CLI's own half**, since the provider has no hook for either. `PlayerListener` writes through on every server change, and startup seeds the sink, the @@ -1663,9 +1663,9 @@ locally settable, and volume changes are ramped. Split out of item 4. **This item's opening premise was wrong, and correcting it is part of what shipped.** It claimed `set_static_delay_adjustable(true)` was advertised with no `on_static_delay_changed()` override, so "a controller can offer the user a static delay this player then applies to nothing". Against -sendspin-cpp v0.7.0 that is not true, and it never was: `SyncTask::decode_chunk()` subtracts +sendspin-cpp v0.8.0 that is not true, and it never was: `SyncTask::decode_chunk()` subtracts `get_effective_static_delay_ms()` from every chunk's client timestamp -(`_deps/sendspin-src/src/sync_task.cpp:593`), and that value becomes `decoded_timestamp`, which +(`_deps/sendspin-src/src/sync_task.cpp:597`), and that value becomes `decoded_timestamp`, which is what `raw_error` is measured against — the drift correction itself. `get_effective_static_delay_ms()` returns the stored delay precisely *because* adjustability is on. So the delay was already being obeyed; the override was never the thing standing between the value and the audio path. A future @@ -1736,7 +1736,7 @@ stdout sink stays a jump. Not an oversight. **The `DEFAULT_SINK_VOLUME` mismatch was already closed, so it was re-documented rather than fixed.** Item 8 made startup call `player.update_volume()`/`update_muted()` unconditionally, with -the volume falling back to `DEFAULT_SINK_VOLUME`, before `start_server()` — so the role and the +the volume falling back to `DEFAULT_SINK_VOLUME`, before `start()` — so the role and the sink agree from before the first `client/state`, and the disagreement four comments still described was no longer observable. Those four (`src/audio_sink.h`, `src/player_listener.h`, `src/control.h`, `src/main.cpp`'s `status()`) now describe the current arrangement, keeping the @@ -1758,7 +1758,7 @@ reasons those members exist: `PlayerListener` is still the only thing that knows **One gap found while scoping this belongs upstream, not here.** `required_lead_time_ms` and `min_buffer_ms` are **REQUIRED** in `client/state` per `roles/player/v1.md` — the server uses them to decide how far ahead to send audio, scheduling the first chunk at least -`min_buffer_ms + static_delay_ms` out — and sendspin-cpp v0.7.0 implements neither anywhere. A +`min_buffer_ms + static_delay_ms` out — and sendspin-cpp v0.8.0 implements neither anywhere. A grep of the whole library, headers and sources, matches neither name. Nothing in this repo can supply them: `ClientPlayerStateObject`, which `PlayerRole::Impl::build_state_fields()` fills, has no field for either. It needs a library change, not a change here. @@ -1877,7 +1877,7 @@ a backoff and a bounded retry, for the backends whose version of it is a server than a device-list rebuild; PortAudio's remains the one attempt described here. No re-advertising of formats mid-session: `capabilities()` is answered once -before `start_server()`, so a rescan does not change what the server was told, and the refusal +before `start()`, so a rescan does not change what the server was told, and the refusal path that already names the device and the format it would not take stays the mitigation, as the comment on `capabilities()` has said since item 3. `NullAudioSink` is untouched, having no device to lose. @@ -1996,14 +1996,14 @@ item, but it should not be lost.** The spec has moved inbound arbitration to an first `server/activate`, with "higher or equal is accepted, lower is rejected", plus a persisted last-*playback* server. -Pinned `sendspin-cpp` v0.7.0 has **no `activities` and no `server/activate` at all**, and +Pinned `sendspin-cpp` v0.8.0 has **no `activities` and no `server/activate` at all**, and still implements the older `connection_reason` DISCOVERY/PLAYBACK handoff. So this item is gated on a library that speaks the newer shape, and is likely to arrive with a `SENDSPIN_GIT_TAG` bump rather than on its own. Item 5's `src/last_server.{h,cpp}` is the nearest thing that exists today and is deliberately named for what it observes — the last server whose *handshake* completed, not -its last *playback* server, which v0.7.0 gives no way to know. +its last *playback* server, which v0.8.0 gives no way to know. ### 18. Native PulseAudio backend — *shipped (audible slice)* @@ -2260,11 +2260,10 @@ no workaround either — the control socket answers questions, it does not annou polled from the main loop, next to mDNS and the control socket. A non-zero exit or a signal death is one `W hook:` line; a hook still running at shutdown is left to finish, because an amplifier half-switched is worse than an orphan. -- **The stream's end is waited for on the way out.** `disconnect()` only enqueues it, and - it is `client.loop()` that delivers it, so without this a player killed mid-stream reaches - `return 0` with its stop hook unrun and the amplifier still on. The loop is pumped after - the disconnect until the listener reports the stream over, bounded by - `SHUTDOWN_DRAIN_MS` against a wait of about fifty, and says so and goes if that passes. +- **The stream's end is waited for on the way out.** `client.stop()` goodbyes every peer and + delivers `on_stream_end()` before it returns, so the hook is queued by the time control comes + back and the `hooks.flush()` below runs it. Without that wait a player killed mid-stream would + reach `return 0` with its stop hook unrun and the amplifier still on. The stop hook it spawns is by definition the orphan case above: nothing waits on it. - **Fired on the stream lifecycle, not on the format being accepted**, through a `PlayerListener::on_stream_event` seam shaped like `AudioSink::on_frames_played`. A @@ -2376,17 +2375,16 @@ detection may pick the wrong interface" — the same scenario, so it is worth sa is not the same lever. That value goes into `client/hello`'s `device_info`: it is identity, and it moves no socket. -**Why this is more than "it listens broadly".** Pinned v0.7.2 has **no inbound +**Why this is more than "it listens broadly".** Pinned v0.8.0 has **no inbound authentication of any kind**: no PSK, no pairing gate on the inbound path, and — per item -17, whose text still cites v0.7.0, re-checked here at v0.7.2 — no `activities` / -`server/activate` either, since it still runs the older `connection_reason` handoff. +17 — no `activities` / `server/activate` either, since it still runs the older `connection_reason` handoff. `server_max_connections` is the only inbound limit, and it counts sockets rather than judging them. Reachability is therefore authorization: whatever reaches the listen port completes the handshake and drives the player, and a bind address would be the only such control the player itself offers. **That is a statement about the pinned library, not about Sendspin.** The spec -authenticates in the handshake — pairing and a PSK, as item 6 records — and v0.7.2 does +authenticates in the handshake — pairing and a PSK, as item 6 records — and v0.8.0 does not implement that half yet. This item's premise expires when it does. **In proportion.** Most installs sit behind NAT, where no interface holds a routable diff --git a/src/audio_sink.h b/src/audio_sink.h index 95e6964..24cced2 100644 --- a/src/audio_sink.h +++ b/src/audio_sink.h @@ -100,14 +100,14 @@ class AudioSink { /// @param now_ms Monotonic milliseconds; derive timing from this, not from counting calls. virtual void poll(int64_t /*now_ms*/) {} - /// What the device will take, probed once before start_server(). + /// What the device will take, probed once before start(). /// An unprobeable device must answer SinkCapabilities::permissive(), never an empty set. virtual SinkCapabilities capabilities() const { return SinkCapabilities::permissive(); } /// Reports frames that reached the DAC, for sync feedback; optional for instant sinks. - /// Assign before start_server() and never after: backends read it from their audio thread. + /// Assign before start() and never after: backends read it from their audio thread. std::function on_frames_played; protected: diff --git a/src/main.cpp b/src/main.cpp index 2cf399e..cb270dd 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -56,9 +56,6 @@ using sendspin::LogLevel; /// Sleep between client.loop() calls; bounds main-loop reaction time only. constexpr int LOOP_INTERVAL_MS = 10; -/// How long shutdown keeps pumping client.loop() for the stream's end (~50 ms needed). -constexpr int SHUTDOWN_DRAIN_MS = 500; - std::atomic g_running{true}; void handle_signal(int /*sig*/) { @@ -600,7 +597,7 @@ int main(int argc, char* argv[]) { sendspin::SendspinClient client(std::move(config)); - // Before add_player() and start_server(), or the library never asks it. + // Before add_player() and start(), or the library never asks it. client.set_persistence_provider(&persistence); std::vector formats = advertised_formats(*sink); @@ -659,7 +656,7 @@ int main(int argc, char* argv[]) { player.update_volume(volume); player.update_muted(muted); - if (!client.start_server()) { + if (!client.start()) { log_fatal(LOG_TAG, "could not start the Sendspin server on port %u", opts.port); return 1; } @@ -668,7 +665,7 @@ int main(int argc, char* argv[]) { SENDSPIN_CLI_VERSION, opts.port, opts.name.c_str(), sink->name().c_str(), mdns_backend_name().c_str()); - // After start_server(), so the advertised port is already accepting. + // After start(), so the advertised port is already accepting. MdnsService mdns; start_advertising(mdns, opts); @@ -736,23 +733,11 @@ int main(int argc, char* argv[]) { } cli_log(LogLevel::INFO, "Shutting down"); - // Before the client disconnects, so a restart does not race a stale record or socket. + // Before the client stops, so a restart does not race a stale record or socket. mdns.stop(); control_socket.close(); - client.disconnect(sendspin::SendspinGoodbyeReason::SHUTDOWN); - // Pump until the stream ends so the stop hook runs; disconnect() only asks. - for (int waited_ms = 0; player_listener.streaming(); waited_ms += LOOP_INTERVAL_MS) { - if (waited_ms >= SHUTDOWN_DRAIN_MS) { - cli_log(LogLevel::WARN, - "The stream did not end within %d ms of disconnecting -- any --hook-stop " - "has not run", - SHUTDOWN_DRAIN_MS); - break; - } - client.loop(); - hooks.poll(); - std::this_thread::sleep_for(std::chrono::milliseconds(LOOP_INTERVAL_MS)); - } + // Returns stopped, having delivered on_stream_end() -- so the stop hook is already queued. + client.stop(); // The stop hook may be pending behind a hung start hook; run it anyway. hooks.flush(); // The lambda references locals destroyed before the listener; drop it first. From 222b7e8531f2acfa64b2165a07fde0041bb6b60f Mon Sep 17 00:00:00 2001 From: Chris Date: Tue, 15 Sep 2026 20:42:09 -0500 Subject: [PATCH 2/3] Update comment for mDNS listener initialization Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index cb270dd..700f3ab 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -665,7 +665,7 @@ int main(int argc, char* argv[]) { SENDSPIN_CLI_VERSION, opts.port, opts.name.c_str(), sink->name().c_str(), mdns_backend_name().c_str()); - // After start(), so the advertised port is already accepting. + // The first client.loop() tick starts the listener after mDNS registration. MdnsService mdns; start_advertising(mdns, opts); From f8ba1c645963cbd68e72fa1f20cf3107e5531890 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 16 Sep 2026 01:43:20 +0000 Subject: [PATCH 3/3] Fix stale roadmap citation Co-authored-by: chrisuthe <26907395+chrisuthe@users.noreply.github.com> --- docs/ROADMAP.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index d953e9a..99dade2 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1681,7 +1681,7 @@ landed: - **`status` reports it**, as `static delay: ms`, read from `PlayerRole::get_static_delay_ms()` rather than from a listener-held shadow. That is not a style choice: `update_static_delay()` does not invoke the listener (only a server's `set_static_delay` does, - `_deps/sendspin-src/src/player_role.cpp:396-400`), so a shadow would be stale the moment the + `_deps/sendspin-src/src/player_role.cpp:403-408`), so a shadow would be stale the moment the local knob below was used. - **`sendspin-cli delay <0-5000>`** sets it locally. The first *mutating* request answered without a server — `status` was previously the only locally answered one at all — which the spec