feat: LMS Connect glue — event dispatch, streaming pipe, token helpers#28
feat: LMS Connect glue — event dispatch, streaming pipe, token helpers#28stiefenm wants to merge 23 commits into
Conversation
Adds the tokio runtime features needed by the LMS-glue layer's TCP notifier (TcpStream::connect + AsyncWriteExt::write_all). Also adds serde_json as an explicit top-level dependency (it is already a transitive workspace dep via librespot-core, but the LMS::notify JSON body builder needs it as a direct dep).
Introduces a new module `librespot::spotty` (gated by the `lms-connect` feature) that holds the LMS-glue layer: - `pub const VERSION = "2.1.0"` — the version label the Spotty-Plugin helper-check regex expects. - `LMS` struct with `host_port`, `player_mac`, `auth`, and a shared `Arc<AtomicBool>` for the suppress-next-volume flag. - `ConnectNullSink` — a real-time-rate-limited audio sink that consumes decoded PCM at SAMPLE_RATE and discards it. Used in Connect-receiver mode where LMS owns the audio path. Critically, `stop()` does NOT exit() the process (unlike StdoutSink) so the daemon survives track transitions. The handle_player_event dispatcher and the notify TCP transport land in the next commit so each commit reads as a single concern. Written from scratch against librespot-org HEAD's audio-backend trait surface (Sink::start/stop/write with SinkResult).
Implements LMS::handle_player_event (the dispatcher) and LMS::notify (the JSON-RPC TCP transport). Wire vocabulary — 5 commands the Phase 8 plugin handler must match: - start : new track playing (None -> Some transition) - change : track id changed (replaces previous) - stop : Paused or Stopped collapse here (NOT `pause`) - volume : 0-100 percent, suppressed once after SessionConnected - seek : position in seconds (3 decimals) Same-id Playing re-emits (post-seek, buffer-underrun) are no-ops so LMS doesn't see a flood of redundant start events. SetQueue and other HEAD-only variants fall through the wildcard arm. Transport: one-shot tokio::net::TcpStream + raw HTTP/1.0 POST to `/jsonrpc.js` on the configured LMS host:port. Failures are logged at WARN; the daemon must never panic on a transient LMS outage. Optional --lms-auth HTTP-Basic header is sent verbatim (caller-encoded).
Adds the wiring half of the LMS-glue layer:
- Three new long-only CLI flags (gated by lms-connect feature):
--lms <host:port>, --lms-auth <base64>, --player-mac <mac>.
- A --check flag that prints the BIN-03 helperCheck-format header
+ capabilities JSON and exits — required by Spotty-Plugin's
Helper::helperCheck regex ("^ok spotty v2.1.0 - using librespot ...").
- ConnectNullSink::open is passed DIRECTLY as the sink builder to
Player::new under #[cfg(feature = "lms-connect")]. The
audio_backend::find registry is NOT mutated; no new backend name is
registered; --backend parsing is untouched (W6-locked direct-pass).
- A Tokio task that pumps the PlayerEvent channel into
LMS::handle_player_event, only when both --lms and --player-mac
are present (LMS::is_configured() == true).
Non-feature builds fall through to the upstream (backend)(device,
format) closure unchanged, so the lms-connect=off path remains pure
librespot-org HEAD.
Add four missing CLI flags required by Spotty-Plugin's OAuth credential save flow (Settings/Callback.pm:358): --get-token authenticate with --access-token + save credentials.json --save-token F same, additionally write credentials to file F --client-id override Spotify client ID for the session --scope accept OAuth scopes (informational with --access-token) The --check handler already claimed save-token:true but the flags were never implemented, causing "Unrecognized option: get-token" at runtime. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fresh When the PKCE OAuth refresh token expires or is revoked by Spotify, the Plugin can now recover automatically by asking the binary for a fresh bearer token via login5 (using stored credentials.json). Adds --keymaster-token CLI flag that connects to Spotify using cached credentials, retrieves an access token via the login5 auth flow, and outputs it as JSON to stdout. Also registers keymaster-token: true in the --check capabilities JSON so the Plugin can gate on availability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TrackChanged was previously ignored ("no LMS equivalent"), but it fires
when librespot loads a new track via Spirc command — e.g. when the user
taps a track further down in the playlist in the Spotify app. Without
dispatching it, the Perl-side Connect handler never learned about
playlist jumps, causing LMS to keep playing the old track.
Now TrackChanged updates the current_track cursor and emits a `change`
(or `start` on first track) event, exactly like the Playing handler.
When Playing fires later for the same track_id, it becomes a same-id
no-op.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Regenerate Cargo.lock after cherry-pick series - Remove duplicate serde_json = "1" from Cargo.toml (already present as serde_json = "1.0") - Remove setup_logging() call in KEYMASTER_TOKEN handler (fn only exists in debug builds)
…ly in spotty.rs When spotty.rs is compiled as a library module (pub mod spotty in lib.rs under cfg(feature = lms-connect)), 'use librespot::core::*' becomes a circular reference since the lib IS librespot. Switch to the direct sub-crate paths (librespot_core::*, librespot_playback::*) which are available as workspace dependencies regardless of context. Also fix to_id() calls: SpotifyUri::to_id() returns Result<String, Error> in Herger's fork. Add .unwrap_or_default() for graceful handling of parse failures. Affects PlayerEvent::Playing and PlayerEvent::TrackChanged handlers in the lms_connect dispatcher. Required for: cross-compilation with --no-default-features --features default-linux,lms-connect Revealed by: aarch64-unknown-linux-musl cross-compile build Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds ChangesLMS Connect Feature Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
We'd previously need a hack somewhere in the playback handler which would not tell Spotify when a track had fully been fed to the sink. This was needed to prevent Spotify from sending the next track within seconds the first one was started. How are you dealing with this situation in this new implementation? |
- Add StdoutStreamSink struct after ConnectNullSink in lms_connect module - Implements Sink trait with rate-limited S16LE PCM stdout writing - stop() resets counters only (no exit) — survives track boundaries (BIN-03) - write() uses tokio::task::block_in_place for sleep (D-02, pitfall S-07) - write() maps broken-pipe errors to SinkError::OnWrite (T-14-04) - add "connect-stream": true to capabilities JSON in check() - add SinkError to lms_connect module imports
…der in main.rs
- Add CONNECT_STREAM constant ("connect-stream") to const block
- Add connect_stream: bool field to Setup struct (cfg lms-connect)
- Add .optflag for --connect-stream in get_setup() opts builder
- Add opt_present(CONNECT_STREAM) to Setup constructor
- Replace hardcoded ConnectNullSink::open with SinkBuilder conditional
selecting StdoutStreamSink::open when setup.connect_stream is true
…ctivates StdoutStreamSink Without this, `cargo build --release` omits the lms-connect feature and the binary falls through to the upstream audio backend rather than StdoutStreamSink. lms-connect must be in the default feature set so the plain build command produces a functional --connect-stream binary. Also added to default-linux profile for parity.
CR-01: Replace tokio::task::block_in_place with plain std::thread::sleep
in StdoutStreamSink::write() — Sink runs on a std::thread, not a
Tokio worker, so block_in_place would panic at runtime.
CR-02: Declare `capabilities` as `let mut` in check() so the
passthrough-decoder cfg block can insert into the JSON map.
WR-01: Reset began_at in stop() to prevent rate-limiter drift across
track boundaries.
WR-03: Flush stdout in stop() to push any kernel-buffered PCM bytes
before track transition.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The old let expected_ns = frames_consumed * 1e9 / SAMPLE_RATE;
if expected_ns > elapsed_ns { sleep(delta) }This means Spirc always reports realistic playback positions to Spotify Cloud, so Spotify never thinks a track finished early. The
|
Hmm... not sure I understand. You'd still only use a |
|
You're right — what's in this PR is still the track-by-track approach. And honestly, it's fragile. Syncing two masters (LMS playlist vs. Spotify session) creates exactly the complexity you warned about. After your feedback on May 21, I took your streaming idea seriously and built a continuous PCM stream mode locally. The binary runs the full Spotify session (Spirc handles track progression, seeking, gapless) and writes a single uninterrupted PCM stream to stdout. LMS just plays it like internet radio — no track-by-track, no playlist sync. This works with the current binary (stdout to a named pipe, LMS reads via It works well for daily use, with two known limitations inherent to the named-pipe transport: ~5-10s progress bar lag after seek, and occasional audio glitch on reconnect. Both would go away with HTTP streaming instead of the pipe — but that would require a binary rebuild (embedded HTTP server in the Rust code). How would you like to proceed? I can update this PR with the stream-mode plugin code so you can look at it. And if you're open to it, building the HTTP transport would be the natural next step. |
|
I'd rather go the HTTP server route - named pipe sounds like trouble on Windows. A new binary would be needed anyway. This sounds like a promising effort! I did give it a quick shot a while ago. But that was before I had Claude available. And it wasn't very successful. And TBH: having this discussion with you shows me that you're serious about this, and that you know what you're doing with the AI. Unless you're an AI yourself 😂. That's an important factor for me to accept somebody's work. |
|
Great, let's go with HTTP streaming. Here's what I'm thinking for the roadmap:
Does that sound right, or would you prefer a different split? |
|
And thanks for the trust. I believe I know what I'm doing, even though I don't have the years of developer background that you and the other contributors here have. |
…reamSink - Add 5 Cargo deps: bytes, http-body-util, hyper 1.x, hyper-util, tokio-stream - Replace StdoutStreamSink with HttpStreamSink (mpsc::Sender<Bytes> + blocking_send) - Add http_stream_server async fn (hyper 1.x, GracefulShutdown, relay-per-connection) - Add "http-stream": true to check() capabilities JSON (BIN-04) - Update main.rs to compile: falls back to ConnectNullSink until Plan 02 wires pcm_tx - cargo check --features lms-connect passes
- Replace ConnectNullSink TODO with full connect_stream branching
- mpsc::channel::<Bytes>(256) created before Player::new; pcm_tx moved into FnOnce closure
- TcpListener::bind("127.0.0.1:0") + println!("stream_port={port}") + stdout flush before event loop (BIN-01/D-02)
- http_stream_server spawned via tokio::spawn; oneshot shutdown channel stored in http_shutdown_tx_opt/http_handle_opt
- spirc_active.store(true) after Spirc::new succeeds; store(false) on spirc_task=None and credentials/session-replace path
- JoinSet extended with HTTP server graceful shutdown task (BIN-06)
- ConnectNullSink::open preserved in else arm for non-streaming Connect mode
CR-01: Add Arc<AtomicBool> relay_active to http_stream_server. A second
connection while a relay is already running now returns 503 Retry-After:1
instead of spawning a concurrent relay that would split the PCM stream.
The relay task clears the flag on exit.
CR-02: In LMS::new, strip embedded CR/LF from the --lms-auth credential
(via replace(['\r', '\n'], "")) to prevent CRLF injection into the
hand-rolled HTTP/1.0 notify request headers.
WR-01: Replace the try_recv/yield_now busy-poll with a
tokio::time::sleep(1ms) fallback. The Mutex cannot be held across an
await point, so try_recv + 1ms sleep is the correct structure.
WR-02: Validate GET method and /stream path in service_fn; return 405
or 404 for any other request before checking spirc_active.
WR-03: Replace .unwrap() on Response builders with .expect("BUG: ...")
to distinguish programming errors from runtime errors.
…WR-05 no-discovery CR-03: Remove the spirc_active.store(true) that fired immediately after Spirc::new (before the Spotify Hello/Welcome handshake). Instead, spawn a dedicated task that subscribes to the player event channel and sets spirc_active=true only when PlayerEvent::SessionConnected is received. This closes the window where the HTTP stream server could return 200 before audio was actually flowing. The store(false) paths (discovery replacement, spirc_task exit) remain in the main select! loop. WR-04: Add a mutual-exclusion check after get_setup(): if both --authenticate and --connect-stream are set, emit an error and exit(1). Combining them causes the daemon to print stream_port=N and then immediately exit after "authorized", leaving LMS with a dead port. WR-05: Add opt_present(CONNECT_STREAM) to the no_discovery_reason gate. --connect-stream is a tightly controlled Connect receiver managed by LMS; allowing Zeroconf advertisement alongside it risks a second Spirc instance being created from the discovery path.
… tokio Runtime Player::new (player.rs:517) spawns a std::thread but calls block_on() with a fresh tokio Runtime inside it, so Sink::write executes within a tokio async context. blocking_send detects the runtime and panics with "Cannot block the current thread from within a runtime". Replace with try_send + spin-retry: the 256-slot channel and rate-limiter keep contention transient. Full → sleep 1ms and retry. Closed → return SinkError::OnWrite for clean exit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Bump VERSION from 2.1.0 to 2.2.0 (TRN-01)
- Add flush_tx: Option<watch::Sender<u64>> and seek_gen: Arc<AtomicU64>
fields to LMS struct; LMS::new takes flush_tx as 4th parameter
- HttpStreamSink gains flush_tx: watch::Sender<u64> field (ownership anchor)
so the channel remains open while the sink is alive; flush signals are
fired from LMS::handle_player_event, not from Sink::start()
- HttpStreamSink::open takes flush_tx as 4th parameter
- http_stream_server gains flush_rx: watch::Receiver<u64> parameter
- Relay task: poll has_changed() at top of each loop iteration; on new
seek generation drain pcm_rx via while rx.try_recv().is_ok() {} (D-03
in-place drain); log drained chunk count at INFO level
- PlayerEvent::Seeked branch: fetch_add(1) seek_gen and fire flush_tx.send()
after JSON-RPC seek notify; fires only on Seeked (not Playing/Stopped)
to preserve gapless track transitions
- Create (flush_tx, flush_rx) = watch::channel::<u64>(0) inside the connect_stream branch alongside (pcm_tx, pcm_rx) - Clone flush_tx for LMS event dispatcher; pass original to HttpStreamSink::open as 4th argument - pcm_rx_opt changed to Option<(pcm_rx, flush_rx)> tuple; http_stream_server call now passes flush_rx as 5th argument - LMS::new call passes flush_tx_for_lms (Some when connect_stream=true, None in headless mode) as 4th argument - flush_tx_for_lms declared as Option<watch::Sender<u64>> before the connect_stream branch so it is accessible to the LMS event dispatcher
After TrackChanged emits 'start', set needs_position_sync flag. The next same-id Playing event sends a 'seek' notification with the actual position_ms so the Perl side can sync the progress bar for mid-song connects. Without this, the position was silently discarded as a "noisy re-emit" — the API poll returns stale progress_ms=0. The flag is an Arc<AtomicBool> shared across clones. It's cleared on change events and after the position sync fires. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.rs (1)
783-809:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate LMS-only flags behind
lms-connect.These options are registered for every
spottybuild, but the correspondingSetupfields and runtime wiring only exist under#[cfg(feature = "lms-connect")]. In aspotty-only build the flags are accepted but ignored, and--connect-streamstill changes behavior later by disabling discovery.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 783 - 809, The options LYRION_MUSIC_SERVER, LMS_AUTH, PLAYER_MAC, KEYMASTER_TOKEN (and any LMS-specific handling of CONNECT_STREAM) are being registered unconditionally even though their corresponding Setup fields and wiring exist only under #[cfg(feature = "lms-connect")]; wrap the registration calls that call .optopt(...) / .optflag(...) for LYRION_MUSIC_SERVER, LMS_AUTH, PLAYER_MAC, KEYMASTER_TOKEN (and any LMS-only handling of CONNECT_STREAM that affects discovery) with cfg(feature = "lms-connect") so these flags are only added when the lms-connect feature is enabled, or alternatively gate the whole options-builder block behind cfg to keep feature builds consistent with the Setup struct. Ensure the exact option identifiers (LYRION_MUSIC_SERVER, LMS_AUTH, PLAYER_MAC, KEYMASTER_TOKEN, CONNECT_STREAM) are the ones gated.src/spotty.rs (1)
69-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdvertise LMS-only capabilities only when
lms-connectis compiled.
check()always reportsconnect-stream,http-stream, andlms-authas available, but the actual implementation for those paths is behind#[cfg(feature = "lms-connect")]below. Aspottybuild without that feature will claim support it doesn't have.Suggested change
let mut capabilities = json!({ "autoplay": true, - "connect-stream": true, "debug": DEBUGMODE, - "http-stream": true, "keymaster-token": true, - "lms-auth": true, "no-ap-port": true, "oauth": true, "podcasts": true, "save-token": true, "temp-dir": true, "version": VERSION, "volume-normalisation": true, "zeroconf-port": true }); + #[cfg(feature = "lms-connect")] + if let Value::Object(map) = &mut capabilities { + map.insert("connect-stream".to_string(), json!(true)); + map.insert("http-stream".to_string(), json!(true)); + map.insert("lms-auth".to_string(), json!(true)); + } + #[cfg(feature = "passthrough-decoder")] if let Value::Object(map) = &mut capabilities { map.insert("ogg-direct".to_string(), json!(true)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spotty.rs` around lines 69 - 84, The capabilities JSON currently unconditionally lists "connect-stream", "http-stream", and "lms-auth" but the implementations are gated by the "lms-connect" feature; update the code so those capability keys are only included when the "lms-connect" feature is enabled (or alternatively, make check() construct capabilities conditionally) — locate the capabilities construction (variable capabilities) and the check() function and wrap insertion of "connect-stream", "http-stream", and "lms-auth" behind #[cfg(feature = "lms-connect")] (or use cfg!(feature = "lms-connect") to push them at runtime) so builds without that feature no longer advertise unsupported capabilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 39-41: The Cargo feature "lms-connect" is being enabled by default
via the "default" and "default-linux" feature lists which forces the LMS runtime
path in src/main.rs for all default builds; remove "lms-connect" from both the
"default" and "default-linux" feature arrays so it remains opt-in, leaving the
other features (e.g., "native-tls"/"rustls-tls-webpki-roots", "spotty",
"with-libmdns") untouched, and ensure the separate "lms-connect" feature entry
remains defined so downstream packagers can opt into it when needed.
In `@src/main.rs`:
- Around line 888-901: The credential-cache lookup for the --keymaster-token
branch currently prefers CACHE over SYSTEM_CACHE; change the precedence to match
the main setup path by trying opt_str(SYSTEM_CACHE) first and then
opt_str(CACHE) so the credential cache (system cache) is used when both are set;
update the closure around cache_dir resolution used by the Cache::new call
(symbols: KEYMASTER_TOKEN, CACHE, CACHE_SHORT, SYSTEM_CACHE, Cache::new) so it
sources the path from SYSTEM_CACHE.or_else(|| CACHE) and keeps the same error
handling/exit logic.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 783-809: The options LYRION_MUSIC_SERVER, LMS_AUTH, PLAYER_MAC,
KEYMASTER_TOKEN (and any LMS-specific handling of CONNECT_STREAM) are being
registered unconditionally even though their corresponding Setup fields and
wiring exist only under #[cfg(feature = "lms-connect")]; wrap the registration
calls that call .optopt(...) / .optflag(...) for LYRION_MUSIC_SERVER, LMS_AUTH,
PLAYER_MAC, KEYMASTER_TOKEN (and any LMS-only handling of CONNECT_STREAM that
affects discovery) with cfg(feature = "lms-connect") so these flags are only
added when the lms-connect feature is enabled, or alternatively gate the whole
options-builder block behind cfg to keep feature builds consistent with the
Setup struct. Ensure the exact option identifiers (LYRION_MUSIC_SERVER,
LMS_AUTH, PLAYER_MAC, KEYMASTER_TOKEN, CONNECT_STREAM) are the ones gated.
In `@src/spotty.rs`:
- Around line 69-84: The capabilities JSON currently unconditionally lists
"connect-stream", "http-stream", and "lms-auth" but the implementations are
gated by the "lms-connect" feature; update the code so those capability keys are
only included when the "lms-connect" feature is enabled (or alternatively, make
check() construct capabilities conditionally) — locate the capabilities
construction (variable capabilities) and the check() function and wrap insertion
of "connect-stream", "http-stream", and "lms-auth" behind #[cfg(feature =
"lms-connect")] (or use cfg!(feature = "lms-connect") to push them at runtime)
so builds without that feature no longer advertise unsupported capabilities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3afd839-107c-46c4-aa3c-47c5c9457008
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdCargo.tomlsrc/lib.rssrc/main.rssrc/spotty.rs
| lms-connect = [] # LMS-Glue feature: enables LMS notification dispatch and streaming pipeline | ||
| default = ["native-tls", "spotty", "with-libmdns", "lms-connect"] | ||
| default-linux = ["rustls-tls-webpki-roots", "spotty", "with-libmdns", "lms-connect"] |
There was a problem hiding this comment.
Keep lms-connect opt-in instead of default-enabled.
Adding lms-connect to both default feature sets makes every default spotty build take the LMS-specific runtime path in src/main.rs instead of the pre-existing backend path. That turns a feature described as optional into always-on behavior and changes the binary surface for downstream packagers.
Suggested change
lms-connect = [] # LMS-Glue feature: enables LMS notification dispatch and streaming pipeline
-default = ["native-tls", "spotty", "with-libmdns", "lms-connect"]
-default-linux = ["rustls-tls-webpki-roots", "spotty", "with-libmdns", "lms-connect"]
+default = ["native-tls", "spotty", "with-libmdns"]
+default-linux = ["rustls-tls-webpki-roots", "spotty", "with-libmdns"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lms-connect = [] # LMS-Glue feature: enables LMS notification dispatch and streaming pipeline | |
| default = ["native-tls", "spotty", "with-libmdns", "lms-connect"] | |
| default-linux = ["rustls-tls-webpki-roots", "spotty", "with-libmdns", "lms-connect"] | |
| lms-connect = [] # LMS-Glue feature: enables LMS notification dispatch and streaming pipeline | |
| default = ["native-tls", "spotty", "with-libmdns"] | |
| default-linux = ["rustls-tls-webpki-roots", "spotty", "with-libmdns"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 39 - 41, The Cargo feature "lms-connect" is being
enabled by default via the "default" and "default-linux" feature lists which
forces the LMS runtime path in src/main.rs for all default builds; remove
"lms-connect" from both the "default" and "default-linux" feature arrays so it
remains opt-in, leaving the other features (e.g.,
"native-tls"/"rustls-tls-webpki-roots", "spotty", "with-libmdns") untouched, and
ensure the separate "lms-connect" feature entry remains defined so downstream
packagers can opt into it when needed.
| if opt_present(KEYMASTER_TOKEN) { | ||
| let cache_dir = opt_str(CACHE) | ||
| .or_else(|| opt_str(SYSTEM_CACHE)) | ||
| .unwrap_or_else(|| { | ||
| error!("--{KEYMASTER_TOKEN} requires --{CACHE}/-{CACHE_SHORT}"); | ||
| exit(1); | ||
| }); | ||
|
|
||
| let cache = match Cache::new( | ||
| Some(PathBuf::from(&cache_dir)), | ||
| Some(PathBuf::from(&cache_dir)), | ||
| None, | ||
| None, | ||
| ) { |
There was a problem hiding this comment.
Use the same credential-cache precedence as the main setup path.
--keymaster-token currently prefers --cache over --system-cache, but the normal cache initialization does the opposite for credentials. If both are set, this helper reads the audio cache directory instead of the credential cache and won't find the stored login.
Suggested change
- let cache_dir = opt_str(CACHE)
- .or_else(|| opt_str(SYSTEM_CACHE))
+ let cache_dir = opt_str(SYSTEM_CACHE)
+ .or_else(|| opt_str(CACHE))
.unwrap_or_else(|| {
- error!("--{KEYMASTER_TOKEN} requires --{CACHE}/-{CACHE_SHORT}");
+ error!(
+ "--{KEYMASTER_TOKEN} requires --{SYSTEM_CACHE}/-{SYSTEM_CACHE_SHORT} or --{CACHE}/-{CACHE_SHORT}"
+ );
exit(1);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if opt_present(KEYMASTER_TOKEN) { | |
| let cache_dir = opt_str(CACHE) | |
| .or_else(|| opt_str(SYSTEM_CACHE)) | |
| .unwrap_or_else(|| { | |
| error!("--{KEYMASTER_TOKEN} requires --{CACHE}/-{CACHE_SHORT}"); | |
| exit(1); | |
| }); | |
| let cache = match Cache::new( | |
| Some(PathBuf::from(&cache_dir)), | |
| Some(PathBuf::from(&cache_dir)), | |
| None, | |
| None, | |
| ) { | |
| if opt_present(KEYMASTER_TOKEN) { | |
| let cache_dir = opt_str(SYSTEM_CACHE) | |
| .or_else(|| opt_str(CACHE)) | |
| .unwrap_or_else(|| { | |
| error!( | |
| "--{KEYMASTER_TOKEN} requires --{SYSTEM_CACHE}/-{SYSTEM_CACHE_SHORT} or --{CACHE}/-{CACHE_SHORT}" | |
| ); | |
| exit(1); | |
| }); | |
| let cache = match Cache::new( | |
| Some(PathBuf::from(&cache_dir)), | |
| Some(PathBuf::from(&cache_dir)), | |
| None, | |
| None, | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 888 - 901, The credential-cache lookup for the
--keymaster-token branch currently prefers CACHE over SYSTEM_CACHE; change the
precedence to match the main setup path by trying opt_str(SYSTEM_CACHE) first
and then opt_str(CACHE) so the credential cache (system cache) is used when both
are set; update the closure around cache_dir resolution used by the Cache::new
call (symbols: KEYMASTER_TOKEN, CACHE, CACHE_SHORT, SYSTEM_CACHE, Cache::new) so
it sources the path from SYSTEM_CACHE.or_else(|| CACHE) and keeps the same error
handling/exit logic.
| let chunk = Bytes::copy_from_slice(bytes); | ||
| loop { | ||
| match self.pcm_tx.try_send(chunk.clone()) { | ||
| Ok(()) => break, | ||
| Err(tokio::sync::mpsc::error::TrySendError::Full(_)) => { | ||
| std::thread::sleep(Duration::from_millis(1)); | ||
| } | ||
| Err(tokio::sync::mpsc::error::TrySendError::Closed(_)) => { | ||
| return Err(SinkError::OnWrite( | ||
| "HTTP stream server shut down".into(), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid wedging the sink when no HTTP relay is consuming PCM.
This retry loop never exits while the shared pcm_rx has no active consumer. After a disconnect or reconnect gap, the 256-slot buffer fills and Sink::write blocks forever, which stalls playback instead of degrading gracefully during LMS outages.
2 participants
Context
I use LMS with Spotty daily for my multiroom setup. Two things bugged me enough to dig in and fix them:
Result::unwrap()when switching off phone screen librespot-org/librespot#225: Songs and Home menu should both work regardless of Custom Client ID configurationThis librespot PR is the Rust-side counterpart of PR librespot-org#226 (Connect). The plugin-side Connect code (Connect.pm, Daemon.pm, DaemonManager.pm) manages spotty helper processes and translates Spotify events into LMS commands — but it needs the helper binary to actually speak the Connect protocol, stream audio, and forward events. That is what this lms-connect feature provides.
PR librespot-org#225 (API routing fix) does not require any librespot changes.
Merge order
This PR provides the binary for Spotty-Plugin PR librespot-org#226 (Connect). Merge order: PR librespot-org#225 first, then PR librespot-org#226 and this PR together. PR librespot-org#225 is independent.
Summary
Adds an lms-connect cargo feature that enables the spotty binary to function as a Spotify Connect daemon managed by the LMS Spotty plugin. The binary gains:
Architecture
HttpStreamSink wraps a bounded channel (Arc<Mutex<VecDeque<Vec>>>). An async http_stream_server task accepts connections and streams channel contents as raw S16LE PCM. LMS connects to this HTTP endpoint and plays it as a continuous stream — no reconnect on track change, no FIFO lifecycle management.
The watch-channel seek-flush is layered on top: the relay task holds both the audio receiver and the watch receiver. On flush-generation increment, it drains the audio channel before writing new samples, ensuring stale pre-seek audio never reaches LMS.
Files changed
Pre-built binaries
Pre-built binaries for x86_64 and aarch64 are available in the v2.2.0 pre-release on my fork.
Other platforms can be built with:
Tested on
Summary by CodeRabbit
--lms,--player-mac,--keymaster-token,--connect-stream) for LMS configuration and token management.