Skip to content

chore(sendspin): bump sendspin-cpp pin v0.8.0 → v0.9.3 - #216

Closed
bbangert wants to merge 1 commit into
mainfrom
chore/sendspin-cpp-bump-91038b7
Closed

bbangert wants to merge 1 commit into
mainfrom
chore/sendspin-cpp-bump-91038b7

Conversation

@bbangert

Copy link
Copy Markdown
Owner

Pin

Old 9331ace6428979982c934384702d289958ca125e (v0.8.0)
New 91038b72892596b9ec0cccfec1b9e6ebc58ba148 (v0.9.3)

61 upstream commits. v0.9.3 is the newest tag; main is 9 commits past it (see Deliberately not included below).

⚠️ Draft — does not compile, needs a design decision

Upstream removed SendspinClientConfig::client_id. It is now derived, not configured: the library computes it as base64url(X25519 public key) from a keypair generated on first boot and persisted through a SendspinPersistenceProvider.

Our player wires no persistence provider, and client.cpp:1560 only loads the stored keypair when one is present. So on this pin, as written, the player would mint a fresh keypair — and therefore a fresh 43-char client_id — on every respawn.

That collides with how identity currently flows on our side: Audio.Store generates a client_id per output and persists it, player.ex passes it in as --client-id, and advertises it in the mDNS TXT record as client_id=… (player.ex:542, :692). input/server.ex:893 notes an instance without a matching client_id is silently ignored. After this bump the advertised id and the id the player actually presents in client/hello would no longer agree.

Resolving this is cross-boundary work and a product call, not a mechanical fix, so I stopped rather than guess. Roughly, it needs:

  1. A persistence provider in main.cpp (file-backed, per output) so the keypair is stable across respawns.
  2. An Elixir-side change so the advertised id comes from the player — e.g. the player reports client.client_id() after start() and player.ex advertises that — instead of being supplied to it.
  3. A migration decision: device identity changes once on upgrade regardless of the above, so servers (Music Assistant) will see each output as a new player and lose its settings and group membership. Worth deciding whether that is acceptable or needs a one-time mapping.

--client-id is left in place and still parsed; only the now-removed config assignment at main.cpp:712 is outstanding.

Upstream changes that matter to us

The headline is #140, the Sendspin 1.0.0-rc1 protocol implementation — a wire-format change on an Experimental protocol, which is reason enough to move.

Around it, the connection and receive path was substantially reworked: the receive path now runs on a library-owned protocol task through a shared inbound ring (#148), the connection lifecycle moved onto that task (#149), and the network provider is now called on the main loop only (#189) — all of which touch how our client.loop() driving in main.cpp behaves. Connections now drop cleanly on any send that fails after the Noise encrypt (#163) and disconnects route through drop_connection() (#162), which should make our reconnect path less prone to wedging.

On the player role specifically, audio is now gated on the adopted client/state snapshot (#160), and PlayerRole::get_fixed_delay_us() was removed (#158) — we only ever set fixed_delay_us in config, so that one costs us nothing. The static_delay API was renamed wholesale to output_delay, which is most of the diff in main.cpp here.

Also relevant: IXWebSocket moved to v12.0.1 (#174), and cmake/host.cmake was reworked to let a parent project provide the noise_c target (#185).

Patch 0003 re-validation

Regenerated. Hunk 1 failed against the new source — upstream added <ixwebsocket/*> includes plus <cstddef>/<utility> to the include block, so the anchor context drifted. Hunk 2 (the load-bearing hook after server_->start()) still applied at an offset.

Regenerated against v0.9.3 and verified with patch -p1 --dry-run (exit 0). Both markers CMake asserts are preserved verbatim:

  • sendspin_host_listener_bound(uint16_t port) __attribute__((weak))
  • sendspin_host_listener_bound(port)

Upstream has not absorbed the hook natively, so we still carry it.

Host build

Configure passed (cmake 3.28.3, g++ 13.3.0) — which independently confirms the patch applied cleanly, since assert_patch_markers is fatal at configure time.

libsendspin.a and all vendored deps (microFLAC, micro_opus, noise_c, ixwebsocket) compiled clean. The build then fails only in our own src/main.cpp.

Four of the five breakages were the mechanical static_delay → output_delay rename and are fixed in this PR:

  • on_static_delay_changed → on_output_delay_changed
  • initial_static_delay_ms → initial_output_delay_ms
  • set_static_delay_adjustable → set_output_delay_adjustable
  • get_static_delay_ms → get_output_delay_ms

Our own CLI flag, opts field, and the static_delay stdout event name are unchanged — those are the contract with the Elixir side, not library API.

The fifth is the client_id blocker above. It is the only remaining compile error.

Deliberately not included

main carries 9 commits past v0.9.3, several of which are player-relevant: a hard sync measured from now after the sink starves (#198), Opus decoded at 16 bits regardless of stream/start's bit_depth (#197), player quota released at stream/clear and stream/end (#195), soft-sync frame drops blended into the neighbor (#194), and client/hello sent only after server/hello arrives (#193).

Pinning policy prefers a tag, so this stops at v0.9.3. Worth a follow-up bump once those tag.

Before release

Firmware needs a rebuild (mise run firmware -- <target>) and rpi3 hardware validation — the protocol change and the sync/connection rework are exactly the kind of thing host builds don't exercise. No priv/ binaries or build output are included in this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_014r1JZWiyic2CXqS9R3SeWC


Generated by Claude Code

Upstream landed the Sendspin 1.0.0-rc1 protocol along with player-role
fixes we want: the receive path moved onto a library-owned protocol task,
audio is now gated on the adopted client/state snapshot, and connections
drop cleanly on a failed send. Patch 0003 was regenerated for context
drift in ws_server.cpp's include block.

This bump is NOT mergeable as-is: upstream made client_id a derived value
rather than a configured one, which breaks how the Elixir side supplies
and advertises player identity. See the PR body.

Co-Authored-By: Claude <noreply@anthropic.com>

Copy link
Copy Markdown
Owner Author

CI on c6e5bfc — 5 failures, from two unrelated causes. One is this PR's and already documented; the other is not this PR's and will hit main too.

Not this PR's: Hex audit

##[error]hex.audit found non-allow-listed advisories:
  protobuf 0.17.0 - EEF-CVE-2026-104635 (HIGH)

Uncontrolled recursion in elixir-protobuf/protobuf JSON decoding of self-referential messages (CVE-2026-104635, GHSA-m497-c2h9-rvw6).

This PR changes three C++ files — c_src/sendspin_player/CMakeLists.txt, patches/0003-listener-bound-hook.patch, and src/main.cpp. It touches no mix.exs, no mix.lock, and no Elixir dependency. protobuf is a transitive dep, pulled in by espex 0.11.0 (~> 0.12) and protobuf_generate 0.2.1 (~> 0.17).

The last CI run on main (Oct 6, run 37405603266) was green, so this advisory was published in the interim. main will fail this check on its next run too, as will every open PR — it is a time-based failure, not a code-based one.

I have not spent the one re-run on it: mix hex.audit resolves against a static advisory database, so a re-run reproduces identically by construction. This is not a flake.

No upstream fix PR exists to port, so per convention I'm not widening a C++ pin bump to carry an unrelated dep change. Proposed patch, for a separate PR:

protobuf 0.17.1 is published and satisfies both existing constraints (~> 0.12 and ~> 0.17), so it needs no mix.exs edit:

mix deps.update protobuf

Worth confirming 0.17.1 actually carries the fix before relying on it — I could not retrieve the advisory's first_patched_version from the GitHub Advisory API, and 0.17.1 being the only release after the vulnerable 0.17.0 is suggestive, not proof. If it turns out not to be patched, the fallback is adding EEF-CVE-2026-104635 to the ALLOWED list in ci.yml alongside the two cowlib IDs — though for a HIGH severity that deserves a deliberate decision rather than a default.

This PR's: Compile · Format, Test, Dialyzer, Argus

All four stop on the single error already described in the PR body:

c_src/sendspin_player/src/main.cpp:712:19: error:
  'struct sendspin::SendspinClientConfig' has no member named 'client_id'

(mix compile builds c_src/sendspin_player/, so every job that compiles inherits it. The IXWebSocketPerMessageDeflateCodec.cpp annotations in the same logs are pre-existing upstream warnings, not the stop.)

This is the known blocker: upstream v0.9.3 made client_id derived from a persisted X25519 keypair rather than configured. Fixing it is the identity redesign described in the PR body — a persistence provider, an Elixir-side mDNS change, and a decision on the one-time migration that re-identifies every player. That is a design call, so the PR stays draft and I'm not pushing a speculative fix for it.

Credo and Sobelow passed.


Generated by Claude Code

bbangert commented Oct 19, 2026 •

Copy link
Copy Markdown
Owner Author

Upstream watch, 2026-10-19 — still blocked, and two things have changed

The scheduled upstream watch ran again today. Rather than open a second draft blocked on the same question, here is what moved since this PR was opened. The client_id blocker is unchanged and is still the only thing standing between us and a mergeable bump — it has been open 7 days while upstream kept moving.

1. The commits this PR deferred are now tagged

The Deliberately not included section stopped at v0.9.3 because the player-relevant follow-ups were untagged. All five of them now ship in v0.9.4:

Commit
#193 Send client/hello only after server/hello arrives
#194 Blend the frame soft sync drops into its neighbor
#195 Release the player's quota at stream/clear and stream/end
#197 Decode Opus at 16 bits whatever stream/start's bit_depth says
#198 Measure a hard sync from now after the sink starves

v0.9.5 (b95cf854a784ed308b1f8fe9697f414b02768ddd) adds a player-visible change on top: #203, letting the player withdraw the server's volume and mute commands. The reasoning in this PR for stopping at v0.9.3 no longer applies — the pin should go to v0.9.5 when this lands.

main is 2 commits past v0.9.5 (#206 CI-only, and #208 reading the host clock from CLOCK_MONOTONIC_RAW on Linux — host-relevant to us, but untagged, so policy leaves it).

2. New finding: the source role is silently being compiled in

Not covered by this PR, and worth folding in. Upstream added a source (audio capture) role in the v0.9.x line, and its host build option defaults to ON:

option(SENDSPIN_ENABLE_SOURCE "Enable source role (audio capture)" ON)

Verified identical at both v0.9.3 (this PR's pin) and v0.9.5. Our CMakeLists.txt forces every other unused role OFF but never mentions SENDSPIN_ENABLE_SOURCE, so on this pin we would compile the capture role and its Opus encoder into a binary that only ever plays audio — contrary to the "trim to just the player role" policy the file documents.

Upstream also made the Opus decoder optional in v0.9.0 (SENDSPIN_ENABLE_OPUS, default ON). We advertise Opus in our supported formats, so it should be pinned ON explicitly rather than left to an upstream default that just proved it can change:

set(SENDSPIN_ENABLE_SOURCE     OFF CACHE BOOL "" FORCE)
set(SENDSPIN_ENABLE_OPUS       ON  CACHE BOOL "" FORCE)

I built with those two lines at v0.9.5 and confirmed no source-role objects are produced.

3. Build status at v0.9.5 is identical to this PR's

Reproduced today (cmake 3.28.3, g++ 13.3.0, libasound2-dev installed): configure passes at v0.9.5 — so assert_patch_markers is satisfied and patch 0003 applies. Hunk 1 still needs the same regeneration this PR already made (the anchor drift is in the same include block, and the regenerated patch here applies cleanly at v0.9.5 too, patch -p1 --dry-run exit 0). Both markers are preserved; upstream still has not absorbed the hook natively.

The library and all vendored deps compile clean. The only compile errors are the five in our src/main.cpp — the four static_delay → output_delay renames this PR already fixes, plus client_id.

What is actually needed

Nothing here is a new decision. It is the same one this PR raised: where the X25519 keypair lives so client_id is stable across respawns, and how the mDNS-advertised id comes to agree with the id the player presents in client/hello. Once that is settled, the mechanical work is done and the pin can go straight to v0.9.5 with the two role lines above.

Where today's work lives

I have not pushed to this PR's branch and have not opened a competing PR. Today's findings are on a standalone branch, chore/sendspin-cpp-bump-b95cf85 (commit beff863), pushed only so the work survives — no PR attached, nothing to review unless it is useful to you.

It carries the v0.9.5 pin, the regenerated patch, the two role lines above, and the same four renames as this PR. With those applied, client_id is provably the sole remaining compile error — the build at v0.9.5 now emits exactly one:

c_src/sendspin_player/src/main.cpp:712:19: error: 'struct sendspin::SendspinClientConfig' has no member named 'client_id'

Either cherry-pick it onto this branch or discard it once this PR is updated; it is not meant to compete with this one.


Generated by Claude Code

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.

2 participants