diff --git a/architecture/overview.md b/architecture/overview.md new file mode 100644 index 0000000..7b5e123 --- /dev/null +++ b/architecture/overview.md @@ -0,0 +1,415 @@ +# Crabidy architecture overview + +Status: as-built, describing the workspace at commit `634b89b` (2026-07-20). + +This document records the architecture of the existing system rather than +proposing a new one. Where a design decision is load-bearing, the alternatives +that were genuinely live are recorded alongside it, so a future change can see +what was traded away. + +## Context and problem statement + +Crabidy plays music from a streaming provider (currently Tidal) on the machine +that runs the server, and is driven from a terminal UI that may run elsewhere. +The split exists because the audio hardware and the user are not necessarily in +the same place: the server owns the sound device, the queue, and the provider +credentials; clients are thin and disposable. + +The problems the design has to solve: + +- **Long-lived playback must survive provider failures.** A track that cannot + be fetched, an expired OAuth token, or a queue edit racing with a track + transition must not silently kill playback. This was the dominant historical + bug class. +- **The library is a remote, paginated, slow tree.** Browsing must not block + playback, and playback must not block browsing. +- **Multiple clients observe one shared state.** Queue, play state, volume and + position changes have to reach every connected client. +- **Traces must be attributable.** A command crossing a channel loses its + caller's span by default, which previously made the logs actively misleading. + +## Assumptions + +These are the assumptions the current design rests on. They were inferred from +the code and the deployment shape, not confirmed in discussion — the ones that +would most change the design if wrong are flagged. + +1. **One server instance, one audio device, one user.** There is no + multi-tenancy, no per-client playback, and no authentication on the gRPC + surface. **Load-bearing:** the server binds `0.0.0.0:50051` unauthenticated, + so it assumes a trusted network. +2. **Clients are untrusted only in the sense of being buggy, not hostile.** + Paths from clients are validated for shape but the server will happily act + on any well-formed path. +3. **State is ephemeral.** The queue lives in memory and dies with the process. + `SaveQueue` exists in the proto but is a stub. +4. **Provider count will grow.** The path scheme and the `ProviderClient` trait + are built for more than one provider, though only Tidal is implemented. +5. **The provider is slow and unreliable; the audio device is fast and + reliable.** Timeouts, retries and error recovery are concentrated on the + provider side. + +## Component structure + +Five crates, split so that the wire contract and the provider abstraction do +not depend on either the server or the client. + +```d2 +direction: right + +core: crabidy-core { + explanation: |md + proto (tonic/prost) + ProviderClient trait + path helpers + | +} + +server: crabidy-server { + explanation: |md + gRPC service + playback + provider loops + QueueManager + | +} + +tidaldy: tidaldy { + explanation: |md + Tidal REST client + OAuth device flow + path parsing + | +} + +audio: audio-player { + explanation: |md + rodio wrapper + engine thread + | +} + +tui: cbd-tui { + explanation: |md + ratatui client + | +} + +server -> core: proto + trait +tidaldy -> core: implements ProviderClient +server -> tidaldy: owns instance +server -> audio: commands +tui -> core: proto only +tui -> server: gRPC over TCP +``` + +`crabidy-core` is deliberately dependency-light: it holds the generated proto +types, the `ProviderClient` trait, and the path helpers (`ROOT_PATH`, +`parent_path`, `join_path`, `path_segments`). Both the server and the TUI +depend on it, which is what keeps the wire contract in one place. + +## Runtime structure + +Inside the server process there are three concurrency domains, connected by +channels rather than shared locks. + +```d2 +direction: down + +grpc: tonic gRPC server { + handlers: RPC handlers + stream: GetUpdateStream +} + +playback: Playback loop { + explanation: |md + tokio task + owns QueueManager + owns PlayState + | +} + +provider: ProviderOrchestrator loop { + explanation: |md + tokio task + routes by path prefix + | +} + +bridge: poll_play_bus { + explanation: |md + OS thread + PlayerMessage -> PlaybackCommand + | +} + +engine: PlayerEngine { + explanation: |md + dedicated OS thread + rodio sink + decoder + | +} + +tidal: Tidal REST API +device: Audio device + +grpc.handlers -> playback: PlaybackMessage\nflume bounded(64) +grpc.handlers -> provider: ProviderMessage\nflume bounded(100) +playback -> provider: ResolveTracks / GetTrackUrls\nbounded(1) reply +provider -> tidal: HTTPS, 30s timeout +playback -> engine: PlayerEngineCommand\nflume bounded(16) +engine -> bridge: PlayerMessage +bridge -> playback: PlaybackCommand +engine -> device: PCM +playback -> grpc.stream: StreamUpdate\ntokio broadcast(2048) +``` + +Three properties of this layout matter: + +**The playback loop is the single writer of playback state.** `QueueManager` +and `PlayState` sit behind mutexes inside `Playback`, but only the loop's own +handlers touch them, and no lock is ever held across an `await`. The mutexes +exist because `handle_command` takes `&self`, not because of contention. + +**The audio engine is a blocking OS thread, not a tokio task.** rodio's sink is +synchronous and the decoder does real CPU work, so it would starve the async +runtime. The engine loop uses `recv_timeout(250ms)`: a command wakes it, +otherwise the timeout fires and it emits an elapsed-position tick. It captures +a `tokio::runtime::Handle` at construction so it can `block_on` the HTTP stream +open, and falls back to building its own single-worker runtime if constructed +outside a tokio context. + +**End-of-stream is a callback, not an error.** After the decoder, the engine +appends a `rodio::source::EmptyCallback` carrying a generation number. It fires +only when the decoder ahead of it drains naturally; `reset()` bumps the +generation, so an end-of-stream from a track that was already replaced is +recognised as stale and dropped. This replaced an earlier scheme that detected +end-of-stream by string-matching an `io::Error` message, which lost the signal +whenever the underlying error was anything else — the track would end and +nothing would advance the queue. + +## Path addressing + +Library nodes and tracks are addressed by filesystem-like absolute paths whose +first segment names the provider. The path encodes the position in the tree, so +a parent is obtained by trimming a segment and no separate parent lookup is +needed. + +```d2 +direction: right + +root: "/" { + shape: page +} + +tidal: "/tidal" +playlists: "/tidal/playlists" +playlist: "/tidal/playlists/" +ptrack: "/tidal/playlists//" { + style.fill: "#e8f4e8" +} +artists: "/tidal/artists" +artist: "/tidal/artists/" +album: "/tidal/artists//" +atrack: "/tidal/artists///" { + style.fill: "#e8f4e8" +} + +root -> tidal +tidal -> playlists +playlists -> playlist +playlist -> ptrack +tidal -> artists +artists -> artist +artist -> album +album -> atrack +``` + +Green nodes are track paths. Whether a path is a track or a node is decided by +its **shape**, not by a type tag: `tidaldy::parse_path` matches the segment +slice against a fixed set of variants (`TidalPath::PlaylistTrack`, +`AlbumTrack`, …) and `is_track_path` returns true for exactly the two track +variants. A path that matches no variant is rejected as `MalformedPath` rather +than being guessed at. + +Routing is by prefix. `ProviderOrchestrator` serves `/` itself (a synthetic +root whose only child is `/tidal`) and delegates anything under `/tidal` to the +Tidal client. Adding a provider means adding a prefix arm and a +`ProviderClient` implementation. + +The same path is the unit of queueing. `ResolveTracks` takes any path and +flattens it: a track path yields one track, a node path is walked breadth-first +over its queueable descendants. So "queue this playlist" and "queue this track" +are the same RPC with a different path. + +### Trade-off: paths versus opaque ids + +The previous scheme used opaque ids (`node:playlist:`, `track:`). Paths +were chosen over keeping ids because the tree position is the thing the UI +actually needs — breadcrumbs, "go up", and knowing where a track came from all +fall out of the path for free, and `LibraryNode.parent` becomes derivable +rather than something the provider must remember to populate. + +What this costs: a track's identity is now context-dependent. The same Tidal +track reached through a playlist and through an album has two different paths, +so de-duplication and "is this the track I'm already playing" comparisons +cannot be done by path equality alone. That has not bitten yet because the +queue stores resolved `Track` values, but it is a real constraint on any future +feature that needs stable track identity. + +The proto migration kept field numbers and only renamed `uuid` → `path` and +`uuids` → `paths`, so the change was wire-compatible. + +## Tracing model + +Commands cross channels, and a channel breaks the span parent-child +relationship: by default the handler's events attach to whatever span happened +to be current on the consumer task, which produced logs that attributed work to +unrelated requests. + +Every message is therefore an envelope pairing the command with the sender's +span: + +```rust +pub struct PlaybackMessage { pub span: Span, pub command: PlaybackCommand } +impl PlaybackMessage { + pub fn new(command: PlaybackCommand) -> Self { + Self { span: Span::current(), command } + } +} +``` + +The consumer creates a child of the captured span and instruments the whole +handler future: + +```rust +let handler_span = debug_span!(parent: &span, "playback_command", command = command.name()); +self.handle_command(command).instrument(handler_span).await; +``` + +Instrumenting the *future* (rather than taking an `enter()` guard) is the part +that matters — a guard held across an `await` attributes the time another task +runs to this span. + +The `name()` methods on `PlaybackCommand` and `ProviderCommand` exist to give +the span a low-cardinality label. Paths and positions are recorded as span +fields instead, so per-command spans stay groupable. + +Server logs go to stderr through a non-blocking writer; the TUI cannot share +stderr with its own rendering, so it logs to a rolling daily file under the +state directory. Both honour `RUST_LOG` and default to debug for the workspace +crates, info for everything else. + +## Provider authentication + +Tidal uses the OAuth 2.0 device authorization grant (RFC 8628). The client is +constructed, then `init` tries `login_config` (reuse stored tokens) and falls +back to `login_web` (print a link, poll until authorized). + +```d2 +shape: sequence_diagram + +server: crabidy-server +tidaldy: tidaldy::Client +auth: auth.tidal.com +user: User { shape: person } + +server -> tidaldy: init(toml) +tidaldy -> auth: POST /device_authorization +auth -> tidaldy: device_code, verification_uri, interval +tidaldy -> user: print link +loop: { + tidaldy -> auth: POST /token (secret in body) + auth -> tidaldy: 400 authorization_pending +} +user -> auth: authorizes in browser +tidaldy -> auth: POST /token +auth -> tidaldy: access_token, refresh_token, expires_in +tidaldy -> server: Ok(Client) +``` + +Two details are non-obvious and were the cause of real outages: + +**The client secret travels in the request body, not HTTP Basic auth.** Tidal's +edge rejects Basic auth on `/oauth2/token` with a 403 HTML page. Because the +old poll loop treated every non-success response as "not yet authorized", that +403 was indistinguishable from a normal pending poll, and login appeared to +hang silently for the full five-minute window regardless of whether the user +authorized. The loop now classifies outcomes explicitly — `authorization_pending` +keeps polling, `slow_down` backs off by five seconds per RFC 8628, transport +errors retry, anything else aborts with the provider's actual error text. + +**Access tokens are refreshed proactively.** `ensure_fresh_token` refreshes when +the token expires within five minutes; `make_request` additionally retries once +on a 401 in case the token was revoked or the clock is off. Without this, a +session that outlived its token failed every fetch, and playback died at the +next track boundary with no obvious cause. + +Login state lives behind a `std::sync::RwLock` because the client is shared +immutably while tokens change at runtime. The lock is never held across an +await — a snapshot is cloned out, the request runs, then the result is stored. + +## Boundaries and interfaces + +**Client ↔ server** — gRPC `CrabidyService`. Request/response, except +`GetUpdateStream`, which is server-streaming. + +**RPC → playback** — `flume::bounded(64)` of `PlaybackMessage`. +Fire-and-forget, except `Init`, which carries a reply channel. + +**RPC and playback → provider** — `flume::bounded(100)` of +`ProviderMessage`. Every command carries its own `bounded(1)` reply +channel. + +**Playback → clients** — `tokio::sync::broadcast(2048)`. Lossy by design: +a slow client gets `Status::data_loss` and must resubscribe. + +**Playback ↔ engine** — `flume::bounded(16)` in both directions. Commands +carry reply channels; events flow back unsolicited. + +**Server ↔ provider API** — HTTPS with a 30-second timeout. Failures are +classified into `ProviderError`. + +The broadcast channel is the one place where delivery is deliberately lossy, +and that is surfaced rather than hidden: a lagging subscriber receives an +explicit `data_loss` status instead of quietly missing updates. + +Reply channels are `bounded(1)` and awaited by the caller, which makes each +request/reply pair a rendezvous. This is why the provider loop must never block +indefinitely — the 30-second HTTP timeout is what bounds it. + +## Risks and known gaps + +- **No authentication or transport security.** The gRPC surface is open on + `0.0.0.0:50051`. Fine on a trusted LAN, unsuitable for anything else. +- **The TUI uses unbounded channels** between its UI thread and its RPC task, + which contradicts the workspace's bounded-channel rule. The UI produces + events at human speed so it has not caused problems, but it is unbounded + buffering on a path that can block. +- **The TUI's `orchestrate` task `unwrap`s.** A connection failure after + startup panics that task rather than surfacing an error in the UI. +- **Refreshed tokens are not persisted.** `tidaly.toml` is written once at + startup; tokens refreshed during a session are lost on restart, forcing a new + device login more often than necessary. +- **Mute is unimplemented.** `ToggleMute` is accepted and logged, and the + `MuteChanged` update exists, but the engine has no mute. +- **`SaveQueue` is a stub** that returns success without saving. +- **Single provider.** The abstraction is there but has only one implementation, + so its fit is unproven. +- **The queue is memory-only.** Process restart loses it. + +## Open questions + +1. Should track identity be decoupled from tree position, or is + context-dependent identity acceptable long-term? This decides whether + de-duplication and "already playing" checks are feasible. +2. Does the queue need persistence, and if so is `SaveQueue` the right shape — + named saved queues, or a single autosaved session? +3. Should the server authenticate clients, or is the trusted-network assumption + permanent? +4. When a second provider lands, does prefix routing stay in + `ProviderOrchestrator`, or should providers register themselves? + +## Next stage + +This feeds `api-design` (stage 2). The most likely candidates for API work are +queue persistence and whatever contract a second provider needs. diff --git a/devenv.nix b/devenv.nix index b9d9623..48582d6 100644 --- a/devenv.nix +++ b/devenv.nix @@ -11,6 +11,7 @@ let commonLibs = with pkgs; [ alsa-lib ]; extraPackages = with pkgs; [ + d2 pkg-config protobuf ];