Merge pull request #68 from Sendspin/chrisuthe/task/restore-direct-address-dialling-to-s-alongside

Restore direct-address dialling to -s alongside mdns: discovery

Author: chrisuthe

README.md

@@ -54,16 +54,16 @@ sendspin-cli -n living-room -o hw:1,0
 # Prefer formats, in order -- the server uses the first one it can encode
 sendspin-cli -n living-room --audio-format flac:48000:24:2,pcm:48000:24:2
 
-# Discover a server by its advertised name and connect to it, instead of waiting to be found
-sendspin-cli -n living-room -s "mdns:Music Assistant"
+# Connect out to a specific server, instead of waiting to be found
+sendspin-cli -n living-room -s music.local
 ```
 
 | Option | What it does |
 |---|---|
 | `-n, --name <name>` | The friendly name a server displays. Defaults to this host's name. |
 | `-o, --output <device>` | Which sound card to play through. `-l` lists this host's devices and what they accept. |
 | `--audio-format <codec:rate:depth:channels>[,...]` | Formats to offer first, in priority order, e.g. `flac:48000:24:2,pcm:48000:24:2`. Everything else the player normally offers still follows, and a server uses the first format it can encode, so it can still fall back to a later one. This sets a preference, not a restriction. |
-| `-s, --server mdns:[<name>]` | Discover a server over mDNS and connect to it, rather than waiting to be discovered. `mdns:` takes any server. Turns off the mDNS advertisement. |
+| `-s, --server <host[:port]>` | Connect out to a server rather than waiting to be discovered, or `mdns:[<name>]` to discover one over mDNS. Turns off the mDNS advertisement. |
 
 Any of these can go in a config file instead of on the command line — see
 [Configuration](https://github.com/Sendspin/sendspin-cpp-cli/wiki/Configuration).

docs/ROADMAP.md

@@ -14,9 +14,9 @@ daemonization.
 - Boots a `SendspinClient` with the `player`, `metadata` and `controller` roles, starts its
   WebSocket server, pumps `client.loop()`, and shuts down cleanly on `SIGINT`/`SIGTERM`.
 - Speaks both of the protocol's connection modes, and keeps them exclusive as the spec
-  requires: it advertises `_sendspin._tcp` over mDNS by default, and `-s mdns:` instead
-  discovers a server on `_sendspin-server._tcp` and dials it — retrying with a backoff
-  until it answers (item 5). A typed-in address is no longer accepted (item 26).
+  requires: it advertises `_sendspin._tcp` over mDNS by default, and `-s` instead dials out —
+  either a typed-in address or a `mdns:` server it discovers on `_sendspin-server._tcp` —
+  retrying with a backoff until it answers (item 5, item 26).
 - Defines the `AudioSink` seam (`src/audio_sink.h`) and plays real audio through it:
   auto-detected ALSA (item 2), PortAudio (item 3), PulseAudio (item 18) and PipeWire
   (item 19) backends, with the device-less null/stdout sink as the fallback, so the
@@ -2460,71 +2460,33 @@ CLI can reach around it — so the flag cannot be expressed by any other route.
 on its own. **Not expected to move soon.**
 
 
-### 26. `-s` takes no address — *shipped*
+### 26. Direct-address dialling, removed then restored alongside `mdns:` — *shipped*
 
-`-s` used to take a typed-in address — `<host>[:<port>]` or a `ws://` URL — beside its
-`mdns:` discovery form. The spec has exactly two ways to connect: the server discovers the
+`-s` briefly took only its `mdns:` discovery form: a typed-in `<host>[:<port>]` or `ws://`
+URL was removed because the spec has exactly two ways to connect — the server discovers the
 client through its `_sendspin._tcp` advertisement, or the client discovers the server on
-`_sendspin-server._tcp` and connects "using the advertised address and path". Dialling a
-hand-entered address is neither, so it went.
+`_sendspin-server._tcp` and connects "using the advertised address and path" — and a
+hand-entered address is neither.
+
+That removal is **reversed by choice**: direct-address dialling is restored beside discovery,
+because pointing a player straight at a known server is worth more here than matching the
+spec's connection model exactly. This is a deliberate divergence, not a spec-conformance gap.
 
 **Shipped** in `src/cli.{h,cpp}`, `src/main.cpp`, `src/outbound.{h,cpp}`,
 `tests/cli_test.cpp`, `tests/config_file_test.cpp`, `tests/discovery_test.cpp` and
 `scripts/smoke_test.sh`:
 
-- **`-s mdns:[<name>]` is the only form**, typed or as `server =` in a config file, and it
-  behaves as it did: browse, pick with the last-server tie-break, dial on the retry pacer's
-  backoff, advertisement suppressed.
-- **Anything else is a hard error at parse time**, not a warning, so an install still
-  configured with an address fails loudly rather than quietly changing how it connects. From a
-  config file the error names the file and line, and a bare `mdns` is asked whether it meant
-  `mdns:`.
-- **The error does not quote the value.** An address was where credentials got typed, and
-  `redact_url_userinfo()` went with `parse_server_url()`: a discovered URL is built from a
-  resolved address, a port and a TXT `path`, and has no userinfo to mask. The smoke test's
-  credential-redaction check went with them.
-- **`SENDSPIN_SERVER_URL` is answered only for the server_id the dial chose.** `LastDial` has
-  no "literal URL, taken at its word" case left; a dial with no id answers nothing.
-
-### 28. A second device outage in one stream never recovers — *shipped (hardware pass still owed)*
-
-Found by item 27's CoreAudio hardware pass — two unplugs in one run, the first back in 190 ms,
-the second never — but it belongs to item 14 and sits in the shared `SinkRecovery`, so every
-device-backed sink had it. **A rescan that succeeded retired the budget instead of restoring
-it**: `rescan_done()` sent a recovered rescan and an exhausted one down the same branch, and
-`reopen_spent_` was still set from the first outage. On the second, `reopen_due()` declined,
-`escalate_()` found the rescan spent, and the sink discarded until the next track with no log
-line at all.
-
-**Shipped** in `src/sink_recovery.{h,cpp}`, its callers (`src/alsa_sink.cpp`,
-`src/pulse_sink.cpp`, `src/pipewire_sink.cpp`, `src/portaudio_sink.cpp`) and
-`tests/sink_recovery_test.cpp`:
-
-- **A recovered rescan refills the budget** — the in-place reopen and the whole rescan ladder,
-  back to `SINK_RESCAN_DELAY_MS` — the same intent `reopen_done(true)` already had for its own
-  path. A recovered reopen still refills nothing, so the outage after it goes straight to the
-  rescan, and that rescan's recovery refills both.
-- **The outage gap survives the refill.** Only the attempt bookkeeping refills; the
-  discarded-frame count is not `reset()`'s to drop here, since the first timed write still owes
-  the player that gap, and a device that dies again before one adds to it.
-- **A device that never comes back still gives up.** Within one outage the budget is unchanged —
-  one reopen, then `SINK_RESCAN_ATTEMPTS` rescans on the doubling delay — and only a real
-  recovery refills it.
-- **`rescan_abandoned()` for an attempt that did not recover anything.** The shutdown paths in
-  ALSA, PulseAudio and PipeWire used to report `rescan_done(true)` when `stop()` landed
-  mid-attempt, which would now refill; they abandon instead. PortAudio used to report `true` up
-  front whatever happened; it now abandons on each failure and reports `true` only when the
-  stream is really back, so its rebuild stays one-shot per outage.
-
-**A flapping device is not given a floor of its own.** One that recovers and dies again,
-repeatedly, now keeps recovering — which is what it is doing — and the loop is already paced:
-each outage spends the in-place reopen at most once, the next one has to wait out
-`SINK_RESCAN_DELAY_MS` before its rescan, and a refill never shortens that below the base delay.
-So the main loop pays at most one rescan per two seconds whatever the device does. A cap on
-recoveries per stream would turn a flaky cable back into the silence this item removes.
-
-The regression tests (`ASecondOutageInTheSameStreamRecovers`,
-`EveryOutageAfterARecoveryStillGivesUp`, `TheDiscardedGapSurvivesTheRefillIntoTheNextOutage`)
-fail against the old helper and pass against the new one. **Still owed:** two unplug/replug
-cycles in one stream on real hardware — ALSA first, since it has the most users, then CoreAudio
-with item 27's harness.
+- **`-s <address>` dials directly again**, typed or as `server =` in a config file: a bare
+  host (server default port 8927), `host:port`, a full `ws://`/`wss://` URL, or a bracketed
+  IPv6 literal. `parse_server_url()` resolves it once at parse time, so a bad address fails
+  before the daemon starts.
+- **`-s mdns:[<name>]` still discovers**, unchanged: browse, pick with the last-server
+  tie-break, dial on the retry pacer's backoff. `mdns:` is reserved only before the first
+  colon, so `hifi:8927` is a host and a bare `-s mdns` is a host named `mdns`.
+- **Any `-s` suppresses the advertisement**, address or discovery — the invariant
+  `advertises()` keeps, so no new code path advertises while an address is configured.
+- **A parse error never quotes the credential.** An address can carry userinfo, so
+  `redact_url_userinfo()` masks it out of every message and every line the player logs. The
+  smoke test's credential-redaction check covers the dial line.
+- **`SENDSPIN_SERVER_URL`** answers a discovery dial only for the `server_id` it chose, and a
+  literal-address dial (no id, unverifiable) at its word — `LastDial` carries both cases.

docs/wiki/Advanced-Usage.md

@@ -12,19 +12,18 @@ server to connect:
 sendspin-cli -n living-room
 ```
 
-To make the player find a server and connect to it instead, use `-s`/`--server` with
-`mdns:`. The player browses `_sendspin-server._tcp` and dials what it finds. This disables
+To make the player connect to a known server instead, use `-s`/`--server`. This disables
 mDNS advertisement because the Sendspin protocol does not allow both modes at once:
 
 ```bash
+sendspin-cli --server music.local              # the server port defaults to 8927
+sendspin-cli --server music.local:9000
+sendspin-cli --server ws://music.local:9000/sendspin
+sendspin-cli --server "[2001:db8::1]:8927"     # an IPv6 literal must be bracketed
 sendspin-cli --server mdns:                    # discover any server
 sendspin-cli --server "mdns:Music Assistant"   # ...or one by its advertised name
 ```
 
-`-s` does not take an address. The spec only has a player connect to a server it has
-discovered, so a host, `host:port` or `ws://` URL is refused at startup. Where mDNS cannot
-reach between the two, leave `-s` off and point the server at the player instead.
-
 An outbound connection retries until it answers, and `--mdns-name` is unused in this
 mode. `--no-mdns` turns the advertisement off without switching modes.
 

docs/wiki/Configuration.md

@@ -52,7 +52,7 @@ already does that.
 | `id` | `--id` | the stable client id a server files this player's settings under — two players on one host must not share it | derived from the interface MAC |
 | `manufacturer` | `--manufacturer` | the manufacturer `client/hello` reports | `sendspin-cpp-cli` |
 | `product-name` | `--product-name` | the product name `client/hello` reports | `sendspin-cli` |
-| `server` | `-s`, `--server` | `mdns:` to discover any server, or `mdns:<name>` for the one advertised under that name | none — wait to be discovered |
+| `server` | `-s`, `--server` | `<host>[:<port>]`, a `ws://` URL, or `mdns:[<name>]` | none — wait to be discovered |
 | `port` | `--port` | the port this player's own WebSocket server listens on | `8928` |
 | `buffer-ms` | `--buffer-ms` | audio the output backend keeps queued, 10–2000 | `100` |
 | `audio-format` | `--audio-format` | preferred formats, comma-separated in priority order: `codec:rate:depth:channels[,...]`, e.g. `flac:48000:24:2,pcm:48000:24:2`; offered first in that order, with the rest of the advertised list still behind them, so a server that cannot encode them falls back — a preference, not a restriction; refuses to start if the advertised list does not carry every one — it carries a single channel count — and an `opus` entry at anything but 48000/16 and at most 2 channels is refused outright | none — device-derived order |
@@ -77,6 +77,21 @@ Booleans take `true`/`yes`/`on`/`1` or `false`/`no`/`off`/`0`. A line whose firs
 character is `#` is a comment; a `#` anywhere else is not, so a name or a path is free to
 contain one. Where a key appears twice, the last one wins.
 
+**A `server` URL that carries userinfo belongs here rather than on the command line — and it
+does not authenticate anything.** `ws://user:token@host:8927/sendspin` is accepted, and the
+`user:token` is dropped before the handshake: Sendspin authenticates in the handshake, not in
+the URL, so nothing turns userinfo into an `Authorization` header. If a proxy in front of
+your server wants HTTP Basic, this is not the way to give it to it — and the player accepting
+a credential it cannot send is a wrong it owes a fix, not a feature to configure around.
+
+Where it still matters is what gets written down. A URL typed at `-s` is in the process's
+`argv`, which `ps` shows to **every** local user for as long as the player runs; in this file
+it is protected by the file's own permissions, so `chmod 0600` and an owner is the whole of
+the fix. The player also masks userinfo out of every line *it* writes — `Connecting to
+ws://user:***@host:8927/sendspin` — but every line tagged `sendspin.*` is the library's own
+and prints the URL in full, at the default log level. Treat the log of a `-s` run with
+userinfo in it as sensitive either way.
+
 **Five things cannot come from a file**: `-l`, `-z`, `--config`, `--help` and `--version`.
 Run shape stays on the command line, and a config naming one is refused as an unknown key.
 Excluding them is reversible; debugging a `daemonize` that came out of a file under systemd

docs/wiki/Controlling-the-Player.md

@@ -211,10 +211,12 @@ with — gathered when the stream started, because a stream usually ends when it
 connection goes and there is nothing left to ask by then.
 
 `SENDSPIN_SERVER_URL` is the one to read carefully: it is the URL this run dialled, not
-which server answered. A lost connection clears it, and it is exported only if the stream
-arrived from the discovered server it dialled: `-s` leaves the inbound listener up, and the
-library reports that a connection is up, not where it came from, so the server id is what
-tells a server that dialled *in* from the player's own dial. Read `SENDSPIN_SERVER_ID` when a hook has to be certain which server it is acting
+which server answered. A lost connection clears it, and when discovery chose the server it
+is exported only if the stream arrived from the server it dialled. On a literal `-s URL`
+run the rest is out of the player's reach: `-s` leaves the inbound listener up, and a
+server that dials *in* while that attempt is outstanding or has failed is a connection the
+player cannot tell from its own — the library reports that one is up, not where it came
+from. Read `SENDSPIN_SERVER_ID` when a hook has to be certain which server it is acting
 on.
 
 The hook never blocks playback: it is spawned and reaped from the main loop, its output

docs/wiki/Getting-Started-on-Linux.md

@@ -166,16 +166,14 @@ The player advertises `_sendspin._tcp` and waits. Open your Sendspin controller
 should appear under the name it logged — which is `-n`, falling back to this host's name.
 Nothing needs configuring on the server side.
 
-To go the other way and have the player discover the server and dial it instead, set
-`server` in the config:
+To go the other way and have the player dial the server instead, set `server` in the config:
 
 ```ini
-# The server advertised as "Music Assistant"; `server = mdns:` takes any
-server = mdns:Music Assistant
+# Pick one — a repeated key takes its last value, so keep a single `server` line:
+server = 192.168.1.10          # a host, port 8927 assumed
+#server = mdns:Music Assistant  # or discover one by its advertised name
 ```
 
-There is no address form: the spec only has a player connect to a server it has discovered.
-
 Any `server` value turns the mDNS advertisement off. That is the spec's rule rather than a
 preference here, and the two modes are mutually exclusive by design — see
 [Connection modes](Advanced-Usage#connection-modes).

docs/wiki/Home.md

@@ -26,8 +26,8 @@ I mdns: advertising _sendspin._tcp as "living-room" on port 8928 (path /sendspin
 ```
 
 That is the whole of the usual setup: nothing to configure on either end. A Sendspin
-server discovers the advertisement and dials in. `-s mdns:` inverts it: this player
-discovers a server and dials it, which the protocol treats as the other of two mutually exclusive
+server discovers the advertisement and dials in. `-s <server>` inverts it and makes this
+player the one dialling, which the protocol treats as the other of two mutually exclusive
 modes — see [Connection modes](Advanced-Usage#connection-modes).
 
 Audio goes out through ALSA (the Linux default) or PortAudio (the cross-platform one, and

docs/wiki/Troubleshooting.md

@@ -209,8 +209,8 @@ sudo apt install avahi-daemon libavahi-compat-libdnssd1
 quietly finding nothing:
 
 ```
-I mdns: This build has no mDNS support, so it can neither be discovered nor discover a
-server: point a server at ws://<this-host>:8928/sendspin. See docs/ROADMAP.md.
+I mdns: This build has no mDNS support, so it cannot be discovered: point a server at
+ws://<this-host>:8928/sendspin, or dial one with -s. See docs/ROADMAP.md.
 ```
 
 Rebuild with `libavahi-compat-libdnssd-dev` present, or point the server at the URL by hand.

packaging/sendspin-cli.conf.example

@@ -31,7 +31,8 @@
 # The port this player's own WebSocket server listens on, for a server that connects in.
 #port = 8928
 
-# Discover a server and dial it: mdns: for any, mdns:<name> for one. Disables advertising.
+# Dial a server instead of waiting to be found: <host>[:<port>], a ws:// URL, or mdns:<name> to
+# discover one by its advertised name. Any value here disables advertising, as -s does.
 #server = mdns:Living Room
 
 # Latency added after the audio port, 0-5000 ms. A first-run default: a remembered delay wins.

scripts/smoke_test.sh

@@ -32,6 +32,10 @@ readonly PORT_CONTROL=39285
 readonly PORT_CONTROL_SECOND=39286
 readonly PORT_CONFIG=39287
 readonly PORT_DELAY=39288
+readonly PORT_REDACTION=39289
+
+# Not a phase port: the address the redaction check dials, chosen so nothing answers it.
+readonly PORT_NO_SERVER=39290
 
 readonly MDNS_INSTANCE="sendspin-cli-smoke"
 
@@ -665,6 +669,38 @@ check_static_delay() {
     await_child "$pid" "$EXIT_TIMEOUT_S" >/dev/null 2>&1 || true
 }
 
+# A -s URL carrying credentials is logged with them masked, and no line of ours prints them.
+# The library's own `sendspin.*` lines are excluded: v0.7.0+ logs the dialled URL through a bare
+# fprintf with no sink hook, so nothing here can redact them -- see docs/ROADMAP.md.
+check_credential_redaction() {
+    local log="$WORK_DIR/redaction.log"
+    local out="$WORK_DIR/redaction.out"
+    # Not a real credential, and it never leaves this host: PORT_NO_SERVER answers nothing, so the
+    # dial fails before a byte is sent. On the command line because that is the leak being tested.
+    local secret="s3cr3t-not-a-real-password"
+
+    "$BIN" --no-mdns --no-control -o null --port "$PORT_REDACTION" "${NO_CONFIG[@]}" \
+        -f "$log" -s "ws://smoke:$secret@127.0.0.1:$PORT_NO_SERVER/sendspin" >"$out" 2>&1 &
+    local pid=$!
+    STARTED_PIDS+=("$pid")
+
+    wait_for_line "$log" "Connecting to" "$BOOT_TIMEOUT_S" ||
+        fail "no dial line within ${BOOT_TIMEOUT_S}s. Log: $(cat "$log")"
+
+    # -F, because the mask and the address are both regex metacharacters written literally.
+    grep -qF "Connecting to ws://smoke:***@127.0.0.1:$PORT_NO_SERVER/sendspin" "$log" ||
+        fail "the dial line did not mask the -s userinfo: $(grep 'Connecting to' "$log")"
+
+    local leaked
+    leaked="$(grep -hv ' sendspin\.[^ :]*:' "$log" "$out" | grep -F "$secret" || true)"
+    [ -z "$leaked" ] ||
+        fail "a sendspin-cli log line printed the -s password: $leaked"
+    pass "the dial line masks a -s URL's userinfo, and no line of ours written by then holds it"
+
+    kill -TERM "$pid" 2>/dev/null || true
+    await_child "$pid" "$EXIT_TIMEOUT_S" >/dev/null 2>&1 || true
+}
+
 main() {
     [ -x "$BIN" ] ||
         fail "no executable at '$BIN' -- pass the path to sendspin-cli as the first argument"
@@ -681,6 +717,7 @@ main() {
     check_missing_runtime_dir
     check_static_delay
     check_config_file
+    check_credential_redaction
     printf 'smoke: every check passed\n'
 }