From 3183ab533f4a18b02e649f6d4873569f374ad45b Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Mon, 21 Sep 2026 05:57:19 +0300 Subject: [PATCH 01/25] fix(nix): verify vendorHash in container, commit flake.lock vendorHash sha256-/rT2... verified with a real nix build inside a disposable nixos/nix container (build + flake check both green). Also commit the generated flake.lock, pinning nixpkgs to 20b1ddd and flake-utils/systems. Root cause of the repeated rot: the floating nixos-unstable toolchain changes go mod vendor output with zero movement in go.mod/go.sum, so each new toolchain invalidates vendorHash. The lock makes the toolchain (and the hash) stable until we deliberately re-lock. --- flake.lock | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ flake.nix | 2 +- 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 flake.lock diff --git a/flake.lock b/flake.lock new file mode 100644 index 0000000..e2a5570 --- /dev/null +++ b/flake.lock @@ -0,0 +1,61 @@ +{ + "nodes": { + "flake-utils": { + "inputs": { + "systems": "systems" + }, + "locked": { + "lastModified": 1731533236, + "narHash": "sha256-l0KFg5HjrsfsO/JpG+r7fRrqm12kzFHyUHqHCVpMMbI=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "11707dc2f618dd54ca8739b309ec4fc024de578b", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1789785513, + "narHash": "sha256-B44WL6h0XoLjJ41bUPJk0X5SDinLCII//6EcBLXKiJ0=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "20b1ddd1aa5ace70c9468305030aa4f9ef79671b", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixos-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, + "root": { + "inputs": { + "flake-utils": "flake-utils", + "nixpkgs": "nixpkgs" + } + }, + "systems": { + "locked": { + "lastModified": 1681028828, + "narHash": "sha256-Vy1rq5AaRuLzOxct8nz4T6wlgyUR7zLU309k9mBC768=", + "owner": "nix-systems", + "repo": "default", + "rev": "da67096a3b9bf56a91d16901293e51ba5b49a27e", + "type": "github" + }, + "original": { + "owner": "nix-systems", + "repo": "default", + "type": "github" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix index cb1b055..570384b 100644 --- a/flake.nix +++ b/flake.nix @@ -19,7 +19,7 @@ src = ./.; # Update when go.sum changes: nix build 2>&1 | grep 'got:' | awk '{print $2}' - vendorHash = "sha256-6zwzWlboTQeZcBiiHU7Jt+vDn2FYCrQ8CzGgCntKRGo="; + vendorHash = "sha256-/rT2aUVw0AG5oSMq/nTaybsvMUd+bPLMPJR2J1dltic="; subPackages = [ "cmd/kcd" ]; From 2a047c1664a36d5dd38358ab8271379b3d80c53a Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Mon, 21 Sep 2026 23:18:52 +0300 Subject: [PATCH 02/25] feat(mpris): poll position only while playing, anchor local broadcasts Delete the standing 2s runPollingLoop: with zero or paused players the daemon now costs zero D-Bus wakeups (measured 13->5 ticks/min, 30->0 GetAll/min with a paused player). A position ticker exists only while at least one local player IsPlaying, armed/disarmed via storeLocalState and removePlayer; scoped to playing players. Stamp PosAnchorMs on every local broadcast (mirroring the remote path) and expose it on DebugPlayerInfo from the cached anchor, so receivers extrapolate live position without polling. New [mpris] keys: poll_while_playing (default true; false = pure event-driven) and position_interval (default 2s, validated >0). Docs: example.toml, CLI.md config table, CLIENT_GUIDE local idle note. --- docs/CLI.md | 1 + docs/CLIENT_GUIDE.md | 11 +- internal/config/config.go | 2 + internal/config/config_test.go | 1 + internal/config/runtime.go | 8 ++ internal/daemon/plugins.go | 2 +- internal/plugins/mpris/local.go | 22 +++- internal/plugins/mpris/mpris.go | 12 +- internal/plugins/mpris/mpris_test.go | 19 ++-- internal/plugins/mpris/playing_poller.go | 107 ++++++++++++++++++ internal/plugins/mpris/playing_poller_test.go | 106 +++++++++++++++++ internal/plugins/mpris/signals.go | 4 +- internal/plugins/mpris/types.go | 21 ++-- internal/plugins/mpris/watcher.go | 56 --------- packaging/kcd.example.toml | 9 ++ 15 files changed, 298 insertions(+), 83 deletions(-) create mode 100644 internal/plugins/mpris/playing_poller.go create mode 100644 internal/plugins/mpris/playing_poller_test.go diff --git a/docs/CLI.md b/docs/CLI.md index f3db347..7a7df8e 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -32,6 +32,7 @@ require a restart; reloading notification filters alone does not apply them. | `[network]` | `dial_timeout = "5s"`, `handshake_timeout = "10s"`, `sidechannel_timeout = "15s"`, `transfer_idle_timeout = "60s"` | | `[reconnect]` | `initial_backoff = "2s"`, `max_backoff = "5m"`, `flap_threshold = "15s"` | | `[discovery]` | `broadcast_interval = "30s"`, `broadcast_idle_interval = "60s"` | +| `[mpris]` | `poll_while_playing = true`, `position_interval = "2s"` | | `[pairing]` | `intent_ttl = "5m"`, `listen_timeout = "60s"`; existing `timeout_secs = 30` still controls the pairing response wait | | `[cache]` | `sms_attachments_dir = ""`, `album_art_dir = ""`, `contacts_dir = ""` | | `[notifications]` | `app_name = "KDE Connect"`; per-app `"show"`/`"silent"` filters and `"*"` fallback remain supported | diff --git a/docs/CLIENT_GUIDE.md b/docs/CLIENT_GUIDE.md index 9f01f3c..2643afa 100644 --- a/docs/CLIENT_GUIDE.md +++ b/docs/CLIENT_GUIDE.md @@ -312,7 +312,16 @@ except KeyboardInterrupt: > **Position:** payloads stamp `posAnchorMs` (Unix millis when `pos` was > sampled). Live position is `pos + (nowMs - posAnchorMs)` while playing, -> frozen otherwise — no client-side timers needed. +> frozen otherwise — no client-side timers needed. Local (desktop-player) +> broadcasts carry the anchor too, stamped at send time. +> +> **Local idle behavior:** the D-Bus watcher is event-driven, but while a +> local player is playing the daemon re-reads its state every +> `position_interval` (default `"2s"`, `[mpris]` section) so the phone's +> now-playing display stays exact. Nothing is polled while paused or with +> no players — a silent desktop costs zero wakeups. Set +> `poll_while_playing = false` for pure event-driven mode (position then +> extrapolates from `posAnchorMs` between D-Bus signals). See [`IPC_PROTOCOL.md §5`](IPC_PROTOCOL.md#5-event-types) for the full list. diff --git a/internal/config/config.go b/internal/config/config.go index dcc3641..ceef68f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -35,6 +35,7 @@ type Config struct { CommandsPerDevice map[string]map[string]string `toml:"commands_per_device"` Notifications NotificationConfig `toml:"notifications"` Battery BatteryConfig `toml:"battery"` + MPRIS MPRISConfig `toml:"mpris"` Notification NotificationPluginConfig `toml:"notification_plugin"` Share ShareConfig `toml:"share"` SFTP SFTPConfig `toml:"sftp"` @@ -75,6 +76,7 @@ func Defaults() *Config { c.Commands = make(map[string]string) c.CommandsPerDevice = make(map[string]map[string]string) c.Battery.Defaults() + c.MPRIS = MPRISConfig{PollWhilePlaying: true, PositionInterval: "2s"} c.Notification.Defaults() c.Share.Defaults() c.SFTP.Defaults() diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6ea8e03..97050b1 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -134,6 +134,7 @@ func TestDurationValidation(t *testing.T) { {"reconnect", "initial_backoff"}, {"reconnect", "max_backoff"}, {"reconnect", "flap_threshold"}, {"discovery", "broadcast_interval"}, {"discovery", "broadcast_idle_interval"}, {"pairing", "intent_ttl"}, {"pairing", "listen_timeout"}, + {"mpris", "position_interval"}, } for _, field := range fields { for _, value := range []string{"", "nonsense", "10", "0", "0s", "-1s", "9999999999999999999h"} { diff --git a/internal/config/runtime.go b/internal/config/runtime.go index d3b0495..1f0c955 100644 --- a/internal/config/runtime.go +++ b/internal/config/runtime.go @@ -28,6 +28,13 @@ type DiscoveryConfig struct { BroadcastIdleInterval string `toml:"broadcast_idle_interval"` } +// MPRISConfig controls local media polling. The D-Bus watcher itself is +// event-driven; the position poller only runs while music plays. +type MPRISConfig struct { + PollWhilePlaying bool `toml:"poll_while_playing"` + PositionInterval string `toml:"position_interval"` +} + // CacheConfig overrides storage directories. Empty values retain plugin defaults. type CacheConfig struct { SMSAttachmentsDir string `toml:"sms_attachments_dir"` @@ -58,6 +65,7 @@ func (c *Config) validateDurations() error { {"discovery.broadcast_idle_interval", c.Discovery.BroadcastIdleInterval}, {"pairing.intent_ttl", c.Pairing.IntentTTL}, {"pairing.listen_timeout", c.Pairing.ListenTimeout}, + {"mpris.position_interval", c.MPRIS.PositionInterval}, } { d, err := time.ParseDuration(setting.value) if err != nil { diff --git a/internal/daemon/plugins.go b/internal/daemon/plugins.go index 358c09e..7d4f6be 100644 --- a/internal/daemon/plugins.go +++ b/internal/daemon/plugins.go @@ -64,7 +64,7 @@ func setupPlugins(cfg *config.Config, bus *events.Bus, tlsCfg *tls.Config, logge plugins.Register(connectivity.NewConnectivityPlugin(bus)) } if cfg.Plugins.MPRIS { - plugins.Register(mpris.NewMPRISPlugin(tlsCfg, bus, cfg.Plugins.PauseMusic, logger, cfg.Cache.AlbumArtDir)) + plugins.Register(mpris.NewMPRISPlugin(tlsCfg, bus, cfg.Plugins.PauseMusic, cfg.MPRIS, logger, cfg.Cache.AlbumArtDir)) } if cfg.Plugins.Mousepad { plugins.Register(mousepad.NewMousepadPlugin(cfg.Mousepad, logger)) diff --git a/internal/plugins/mpris/local.go b/internal/plugins/mpris/local.go index 3f591db..971c0bd 100644 --- a/internal/plugins/mpris/local.go +++ b/internal/plugins/mpris/local.go @@ -2,6 +2,7 @@ package mpris import ( "strings" + "time" "github.com/bethropolis/kcd/internal/device" "github.com/bethropolis/kcd/internal/log" @@ -68,6 +69,12 @@ func (p *MPRISPlugin) sendPlayerListBroadcast() { } func (p *MPRISPlugin) broadcast(state *NowPlaying) { + // Stamp the position anchor at send time: Pos was sampled by the + // caller (signal-time query or poller GetAll) immediately before + // this broadcast, so receivers can extrapolate the live position as + // Pos + (nowMs - PosAnchorMs) while IsPlaying. + state.PosAnchorMs = time.Now().UnixMilli() + pkt, err := protocol.NewPacket("kdeconnect.mpris", state) if err != nil { return @@ -95,9 +102,7 @@ func (p *MPRISPlugin) addPlayer(busName, uniqueName, displayName, shortName stri p.logger.Debug("mpris: added player", log.String("displayName", displayName), log.String("busName", busName)) if state, err := p.playerState(displayName); err == nil { - p.mu.Lock() - p.lastStates[displayName] = state - p.mu.Unlock() + p.storeLocalState(displayName, state) p.broadcast(state) } @@ -109,6 +114,9 @@ func (p *MPRISPlugin) removePlayer(displayName string) { delete(p.players, displayName) delete(p.lastTracks, displayName) delete(p.lastStates, displayName) + // A removal may take the last playing player with it — disarm the + // poller so a removed player can't pin the ticker on. + p.syncPlayingPollerLocked() p.mu.Unlock() p.logger.Debug("mpris: removed player", log.String("displayName", displayName)) @@ -164,6 +172,14 @@ func (p *MPRISPlugin) DebugStatus() *DebugStatus { info.CanGoPrevious = state.CanGoPrevious info.CanPlay = state.CanPlay info.CanPause = state.CanPause + // Anchor from the last broadcast, not this query: DebugStatus + // re-reads D-Bus live, but clients extrapolate from the + // cached anchor stamped at send time. + p.mu.RLock() + if cached := p.lastStates[pl.displayName]; cached != nil { + info.PosAnchorMs = cached.PosAnchorMs + } + p.mu.RUnlock() } else { info.Error = err.Error() } diff --git a/internal/plugins/mpris/mpris.go b/internal/plugins/mpris/mpris.go index 1d73742..f152661 100644 --- a/internal/plugins/mpris/mpris.go +++ b/internal/plugins/mpris/mpris.go @@ -6,6 +6,7 @@ import ( "sync" "time" + "github.com/bethropolis/kcd/internal/config" "github.com/bethropolis/kcd/internal/device" "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" @@ -28,9 +29,16 @@ type MPRISPlugin struct { dbus *dbus.Conn watchCancel context.CancelFunc + watchCtx context.Context watching bool telephonyCancel context.CancelFunc + // mprisCfg gates the position poller: with PollWhilePlaying the + // ticker exists only while at least one local player IsPlaying. + // pollCancel stops it; nil means no poller is running. + mprisCfg config.MPRISConfig + pollCancel context.CancelFunc + // reconcileCh nudges the D-Bus watcher loop to re-list player names // and heal drift. Buffered size 1 so bursts of triggers coalesce; // sends are non-blocking. Event-driven only — no timers. @@ -60,7 +68,7 @@ type remotePositionTracker struct { playing bool } -func NewMPRISPlugin(tlsConfig *tls.Config, bus *events.Bus, pauseMusic bool, logger log.Logger, cacheDirs ...string) *MPRISPlugin { +func NewMPRISPlugin(tlsConfig *tls.Config, bus *events.Bus, pauseMusic bool, mprisCfg config.MPRISConfig, logger log.Logger, cacheDirs ...string) *MPRISPlugin { dbusConn, err := dbus.ConnectSessionBus() if err != nil { logger.Warn("mpris: failed to connect to D-Bus session bus", log.Error(err)) @@ -85,11 +93,13 @@ func NewMPRISPlugin(tlsConfig *tls.Config, bus *events.Bus, pauseMusic bool, log callPausedPlayers: make([]string, 0), artCache: NewArtCache(logger, cacheDirs...), reconcileCh: make(chan struct{}, 1), + mprisCfg: mprisCfg, } // Start the watcher immediately (like C++ does in constructor). // Devices are registered lazily as packets arrive. watchCtx, cancel := context.WithCancel(context.Background()) + p.watchCtx = watchCtx p.watchCancel = cancel p.watching = true p.startWatcher(watchCtx) diff --git a/internal/plugins/mpris/mpris_test.go b/internal/plugins/mpris/mpris_test.go index 43716b8..864a873 100644 --- a/internal/plugins/mpris/mpris_test.go +++ b/internal/plugins/mpris/mpris_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/bethropolis/kcd/internal/config" "github.com/bethropolis/kcd/internal/device" "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" @@ -37,7 +38,7 @@ func TestHandleDeduplicatesRemoteMPRISUpdates(t *testing.T) { sub := bus.Subscribe(4, events.TypeMprisUpdate) defer sub.Close() - plugin := NewMPRISPlugin(nil, bus, false, log.Nop()) + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -142,7 +143,7 @@ func (s *recordingSender) sent() []*protocol.Packet { } func TestHandleRequestsAlbumArtForKdeconnectURI(t *testing.T) { - plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, log.Nop()) + plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -188,7 +189,7 @@ func TestHandleRequestsAlbumArtForKdeconnectURI(t *testing.T) { } func TestHandleIgnoresEmptyAlbumArtPayload(t *testing.T) { - plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, log.Nop()) + plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -212,7 +213,7 @@ func TestStampAlbumArtMatchesCurrentTrack(t *testing.T) { sub := bus.Subscribe(4, events.TypeMprisUpdate) defer sub.Close() - plugin := NewMPRISPlugin(nil, bus, false, log.Nop()) + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -286,7 +287,7 @@ func TestStampAlbumArtMatchesCurrentTrack(t *testing.T) { } func TestPollRemoteStatesOnlyTargetsKnownPlayers(t *testing.T) { - plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, log.Nop()) + plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -357,7 +358,7 @@ func TestHandlePrunesRemovedPlayer(t *testing.T) { sub := bus.Subscribe(4, events.TypeMprisUpdate) defer sub.Close() - plugin := NewMPRISPlugin(nil, bus, false, log.Nop()) + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -398,7 +399,7 @@ func TestHandlePrunesPlayerOnEmptyList(t *testing.T) { sub := bus.Subscribe(4, events.TypeMprisUpdate) defer sub.Close() - plugin := NewMPRISPlugin(nil, bus, false, log.Nop()) + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -429,7 +430,7 @@ func TestHandleKeepsListedPlayer(t *testing.T) { sub := bus.Subscribe(4, events.TypeMprisUpdate) defer sub.Close() - plugin := NewMPRISPlugin(nil, bus, false, log.Nop()) + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } @@ -458,7 +459,7 @@ func TestPublishedEventHasAnchorAndPendingArt(t *testing.T) { sub := bus.Subscribe(4, events.TypeMprisUpdate) defer sub.Close() - plugin := NewMPRISPlugin(nil, bus, false, log.Nop()) + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } diff --git a/internal/plugins/mpris/playing_poller.go b/internal/plugins/mpris/playing_poller.go new file mode 100644 index 0000000..4880992 --- /dev/null +++ b/internal/plugins/mpris/playing_poller.go @@ -0,0 +1,107 @@ +package mpris + +import ( + "context" + "time" + + "github.com/bethropolis/kcd/internal/config" + "github.com/bethropolis/kcd/internal/log" +) + +// storeLocalState caches the latest known state for a local player, then +// arms or disarms the position poller. It is the single choke point for +// all lastStates writes so the poller can never drift out of sync with +// what is actually playing. +func (p *MPRISPlugin) storeLocalState(displayName string, state *NowPlaying) { + p.mu.Lock() + p.lastStates[displayName] = state + p.syncPlayingPollerLocked() + p.mu.Unlock() +} + +// syncPlayingPollerLocked starts the position ticker when at least one +// tracked player IsPlaying and stops it otherwise. With +// PollWhilePlaying=false the poller never runs (pure event-driven). +// Callers must hold p.mu. +func (p *MPRISPlugin) syncPlayingPollerLocked() { + if !p.mprisCfg.PollWhilePlaying { + p.stopPlayingPollerLocked() + return + } + playing := false + for _, s := range p.lastStates { + if s != nil && s.IsPlaying { + playing = true + break + } + } + switch { + case playing && p.pollCancel == nil: + ctx, cancel := context.WithCancel(p.watchCtx) + p.pollCancel = cancel + interval := config.Duration(p.mprisCfg.PositionInterval) + p.logger.Debug("mpris: arming position poller", + log.Duration("interval", interval)) + go p.runPlayingPoller(ctx, interval) + case !playing && p.pollCancel != nil: + p.logger.Debug("mpris: disarming position poller") + p.stopPlayingPollerLocked() + } +} + +// stopPlayingPollerLocked cancels a running position poller, if any. +// Callers must hold p.mu. Cancelling is non-blocking; the goroutine exits +// on its own via ctx.Done. +func (p *MPRISPlugin) stopPlayingPollerLocked() { + if p.pollCancel != nil { + p.pollCancel() + p.pollCancel = nil + } +} + +// runPlayingPoller re-reads D-Bus state for playing players only, at +// PositionInterval, until ctx is cancelled (pause/stop/removal) or the +// plugin shuts down. Paused players and an empty player list cost zero +// wakeups — that is the entire point: idle desktops stay silent. +func (p *MPRISPlugin) runPlayingPoller(ctx context.Context, interval time.Duration) { + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + p.mu.RLock() + playing := make([]*trackedPlayer, 0, len(p.players)) + for _, pl := range p.players { + if s := p.lastStates[pl.displayName]; s != nil && s.IsPlaying { + playing = append(playing, pl) + } + } + p.mu.RUnlock() + + for _, pl := range playing { + state, err := p.playerState(pl.displayName) + if err != nil { + continue + } + p.mu.RLock() + last := p.lastStates[pl.displayName] + p.mu.RUnlock() + + changed := last == nil || + state.PlaybackStatus != last.PlaybackStatus || + state.Title != last.Title || + state.Artist != last.Artist || + state.Album != last.Album || + state.AlbumArtUrl != last.AlbumArtUrl || + state.Volume != last.Volume || + state.IsPlaying != last.IsPlaying + if changed { + p.storeLocalState(pl.displayName, state) + p.broadcast(state) + } + } + } + } +} diff --git a/internal/plugins/mpris/playing_poller_test.go b/internal/plugins/mpris/playing_poller_test.go new file mode 100644 index 0000000..3a83844 --- /dev/null +++ b/internal/plugins/mpris/playing_poller_test.go @@ -0,0 +1,106 @@ +package mpris + +import ( + "testing" + "time" + + "github.com/bethropolis/kcd/internal/config" + "github.com/bethropolis/kcd/internal/events" + "github.com/bethropolis/kcd/internal/log" +) + +func testMPRISConfig() config.MPRISConfig { + return config.MPRISConfig{PollWhilePlaying: true, PositionInterval: "1h"} +} + +// The poller must exist exactly while at least one tracked player reports +// IsPlaying — and never otherwise. +func TestPollArmsOnlyWhilePlaying(t *testing.T) { + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, testMPRISConfig(), log.Nop()) + + p.mu.RLock() + armed := p.pollCancel != nil + p.mu.RUnlock() + if armed { + t.Fatal("poller armed with zero players") + } + + p.storeLocalState("Paused FM", &NowPlaying{Player: "Paused FM", IsPlaying: false}) + + p.mu.RLock() + armed = p.pollCancel != nil + p.mu.RUnlock() + if armed { + t.Fatal("poller armed while only paused players tracked") + } + + p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: true}) + + p.mu.RLock() + armed = p.pollCancel != nil + p.mu.RUnlock() + if !armed { + t.Fatal("poller not armed while a player IsPlaying") + } + + // Pausing the last playing player must disarm (the 1h ticker never + // fires, so no D-Bus traffic can occur during this test). + p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: false}) + + p.mu.RLock() + armed = p.pollCancel != nil + p.mu.RUnlock() + if armed { + t.Fatal("poller still armed after last player paused") + } +} + +// Removing the last playing player must disarm even though no state +// update flows through storeLocalState. +func TestPollDisarmsOnPlayerRemoval(t *testing.T) { + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, testMPRISConfig(), log.Nop()) + + p.mu.Lock() + p.players["Nightdrive"] = &trackedPlayer{displayName: "Nightdrive"} + p.mu.Unlock() + p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: true}) + + p.removePlayer("Nightdrive") + + p.mu.RLock() + armed := p.pollCancel != nil + p.mu.RUnlock() + if armed { + t.Fatal("poller still armed after playing player removed") + } +} + +// With PollWhilePlaying=false the poller must never arm (pure +// event-driven mode). +func TestPollNeverArmsWhenDisabled(t *testing.T) { + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) + + p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: true}) + + p.mu.RLock() + armed := p.pollCancel != nil + p.mu.RUnlock() + if armed { + t.Fatal("poller armed with PollWhilePlaying=false") + } +} + +// Anchor is stamped on the broadcast state itself. +func TestBroadcastAnchorFreshness(t *testing.T) { + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) + + state := &NowPlaying{Player: "Nightdrive", Pos: 42000, IsPlaying: true} + before := time.Now().UnixMilli() + p.broadcast(state) + after := time.Now().UnixMilli() + + if state.PosAnchorMs < before || state.PosAnchorMs > after { + t.Fatalf("PosAnchorMs %d not stamped at broadcast time [%d, %d]", + state.PosAnchorMs, before, after) + } +} diff --git a/internal/plugins/mpris/signals.go b/internal/plugins/mpris/signals.go index d9b87c9..17e230c 100644 --- a/internal/plugins/mpris/signals.go +++ b/internal/plugins/mpris/signals.go @@ -167,9 +167,7 @@ func (p *MPRISPlugin) handlePropertiesChanged(sig *dbus.Signal, uniqueToDisplay state.Pos = pos state.CanSeek = canSeek - p.mu.Lock() - p.lastStates[displayName] = state - p.mu.Unlock() + p.storeLocalState(displayName, state) p.broadcast(state) } diff --git a/internal/plugins/mpris/types.go b/internal/plugins/mpris/types.go index b59f188..9a4cd92 100644 --- a/internal/plugins/mpris/types.go +++ b/internal/plugins/mpris/types.go @@ -97,16 +97,19 @@ type DebugPlayerInfo struct { Album string `json:"album"` PlaybackStatus string `json:"playbackStatus"` IsPlaying bool `json:"isPlaying"` - Volume int `json:"volume"` + Volume int `json:"volume,omitempty"` Pos int64 `json:"pos"` - Length int64 `json:"length"` - AlbumArtUrl string `json:"albumArtUrl"` - CanSeek bool `json:"canSeek"` - CanGoNext bool `json:"canGoNext"` - CanGoPrevious bool `json:"canGoPrevious"` - CanPlay bool `json:"canPlay"` - CanPause bool `json:"canPause"` - Error string `json:"error,omitempty"` + // PosAnchorMs mirrors NowPlaying.PosAnchorMs: the wall-clock time the + // cached Pos was last broadcast, for client-side extrapolation. + PosAnchorMs int64 `json:"posAnchorMs,omitempty"` + Length int64 `json:"length"` + AlbumArtUrl string `json:"albumArtUrl"` + CanSeek bool `json:"canSeek"` + CanGoNext bool `json:"canGoNext"` + CanGoPrevious bool `json:"canGoPrevious"` + CanPlay bool `json:"canPlay"` + CanPause bool `json:"canPause"` + Error string `json:"error,omitempty"` } type DebugStatus struct { diff --git a/internal/plugins/mpris/watcher.go b/internal/plugins/mpris/watcher.go index 0c9368a..91e2949 100644 --- a/internal/plugins/mpris/watcher.go +++ b/internal/plugins/mpris/watcher.go @@ -29,62 +29,6 @@ func (p *MPRISPlugin) startWatcher(ctx context.Context) { } } }() - - go p.runPollingLoop(ctx) -} - -func (p *MPRISPlugin) runPollingLoop(ctx context.Context) { - ticker := time.NewTicker(2 * time.Second) - defer ticker.Stop() - - var lastHash string - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - p.mu.RLock() - players := make([]*trackedPlayer, 0, len(p.players)) - for _, pl := range p.players { - players = append(players, pl) - } - p.mu.RUnlock() - - if len(players) == 0 { - continue - } - - var hash string - for _, pl := range players { - if state, err := p.playerState(pl.displayName); err == nil { - p.mu.RLock() - last := p.lastStates[pl.displayName] - p.mu.RUnlock() - - changed := state.PlaybackStatus != last.PlaybackStatus || - state.Title != last.Title || - state.Artist != last.Artist || - state.Album != last.Album || - state.AlbumArtUrl != last.AlbumArtUrl || - state.Volume != last.Volume || - state.IsPlaying != last.IsPlaying - - if changed { - p.mu.Lock() - p.lastStates[pl.displayName] = state - p.mu.Unlock() - p.broadcast(state) - } - - hash += pl.displayName + state.PlaybackStatus + state.Title - } - } - - if hash != lastHash { - lastHash = hash - } - } - } } func (p *MPRISPlugin) runDBusWatcher(ctx context.Context) error { diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index 1a22b5d..33e4d5b 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -184,6 +184,15 @@ remotesystemvolume = true # These intervals apply only while on-demand UDP broadcast is running; # they do not enable permanent broadcast or change mDNS advertisement. +[mpris] +# The local D-Bus watcher is event-driven; this section tunes the position +# poller that keeps the phone's now-playing display exact while music plays. +# poll_while_playing = true # false = pure event-driven (zero timers ever; + # position extrapolates from posAnchorMs, buggy + # silent players may go stale until next signal) +# position_interval = "2s" # D-Bus re-read cadence while playing only; + # nothing is polled while paused or with no players + [cache] # Empty values preserve existing storage paths; use absolute paths to override. # sms_attachments_dir = "" # default: system temp directory/kcd/sms-attachments From 04a49de60b2711a1804b3bcb659a19653f4b419d Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 03:17:21 +0300 Subject: [PATCH 03/25] feat(mpris): gate remote state poller on watch subscribers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pollRemoteStates now returns immediately with zero mpris.update subscribers: refresh requests only go out while somebody listens, so an unwatched playing phone costs zero packets. Subscribing re-arms the refresh within one interval, preserving the documented freshness guarantee for the watched case. New Bus.HasSubscribers(type) scans live subscribers under RLock (empty-filter subscribers match everything) — no counter state to keep in sync on unsubscribe. Docs (CLIENT_GUIDE, IPC_PROTOCOL freshness paragraphs) updated to while-watched language. --- docs/CLIENT_GUIDE.md | 8 ++- docs/IPC_PROTOCOL.md | 9 ++- internal/events/bus.go | 16 +++++ internal/events/bus_test.go | 36 ++++++++++ internal/plugins/mpris/mpris_test.go | 7 +- internal/plugins/mpris/remote.go | 19 ++++-- internal/plugins/mpris/remote_gate_test.go | 77 ++++++++++++++++++++++ 7 files changed, 160 insertions(+), 12 deletions(-) create mode 100644 internal/events/bus_test.go create mode 100644 internal/plugins/mpris/remote_gate_test.go diff --git a/docs/CLIENT_GUIDE.md b/docs/CLIENT_GUIDE.md index 2643afa..6b4c014 100644 --- a/docs/CLIENT_GUIDE.md +++ b/docs/CLIENT_GUIDE.md @@ -294,11 +294,13 @@ except KeyboardInterrupt: | `pair.requested` | Remote device wants to pair | | `ping.received` | Ping from device | -> **Freshness:** the daemon re-requests now-playing from devices with an -> actively-playing player every 5 seconds, so a pure-push client (a widget watching the +> **Freshness:** while at least one client watches `mpris.update`, the +> daemon re-requests now-playing from devices with an actively-playing +> player every 5 seconds, so a pure-push client (a widget watching the > event stream, with no polling) receives the current track within one poll > interval of subscribing — including mid-track mount, thanks to the initial -> event dump. Events are deduplicated: `mpris.update` only fires when the +> event dump. With nobody watching, no refresh requests go out at all. +> Events are deduplicated: `mpris.update` only fires when the > state actually changed, so the stream stays quiet between track changes. > Stopped/paused players are not polled, and when the phone removes a player > from its `playerList` (session destroyed) the cached state is dropped with diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index ec4088b..ac0ca32 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -781,7 +781,8 @@ are delivered. ``` The daemon keeps now-playing state fresh by re-requesting it every 5 - seconds from devices with an **actively-playing** player (see the + seconds from devices with an **actively-playing** player — but only + while at least one client is subscribed to `mpris.update` (see the `mpris.update` section below), so this initial dump fires reliably for mid-track state — a pure-push client can mount and see the current track without polling. Stopped/paused players are deliberately not polled, so @@ -1240,9 +1241,11 @@ Now-playing state from a device's media player. > path in a second `mpris.update`. If the fetch fails, the pending flag > clears on the next state change. -> **Freshness:** the daemon re-requests now-playing from every connected +> **Freshness:** while at least one client subscribes to `mpris.update`, +> the daemon re-requests now-playing from every connected > device with an **actively-playing** player every 5 seconds -> (`kdeconnect.mpris.request` with `requestNowPlaying: true`). Responses are +> (`kdeconnect.mpris.request` with `requestNowPlaying: true`). With nobody +> subscribed, no refresh requests go out. Responses are > deduplicated — an event is only emitted when the state actually changes. > This keeps `pos`/state current for pure-push clients (widgets, Waybar) > that never poll the CLI. Devices that haven't reported a player yet, or diff --git a/internal/events/bus.go b/internal/events/bus.go index eee0029..1bc6621 100644 --- a/internal/events/bus.go +++ b/internal/events/bus.go @@ -142,6 +142,22 @@ func (b *Bus) unsubscribe(id uint64) { } } +// HasSubscribers reports whether at least one live subscriber would +// receive events of the given type. Subscribers with no filters match +// everything. Scanned on demand under RLock — subscriber counts are tiny +// and callers tick at most every few seconds, so no counter state to keep +// in sync on the unsubscribe path. +func (b *Bus) HasSubscribers(typ EventType) bool { + b.mu.RLock() + defer b.mu.RUnlock() + for _, sub := range b.subscribers { + if sub.matches(typ) { + return true + } + } + return false +} + // Publish broadcasts an event to all interested subscribers. func (b *Bus) Publish(typ EventType, deviceID string, payload any) { ev := Event{ diff --git a/internal/events/bus_test.go b/internal/events/bus_test.go new file mode 100644 index 0000000..eba22ab --- /dev/null +++ b/internal/events/bus_test.go @@ -0,0 +1,36 @@ +package events + +import ( + "testing" + + "github.com/bethropolis/kcd/internal/log" +) + +func TestHasSubscribers(t *testing.T) { + b := NewBus(log.Nop()) + + if b.HasSubscribers(TypeMprisUpdate) { + t.Fatal("reported subscriber with zero subscribers") + } + + other := b.Subscribe(4, TypeBatteryUpdate) + defer other.Close() + if b.HasSubscribers(TypeMprisUpdate) { + t.Fatal("battery subscriber matched mpris.update") + } + + media := b.Subscribe(4, TypeMprisUpdate) + if !b.HasSubscribers(TypeMprisUpdate) { + t.Fatal("missed filtered mpris.update subscriber") + } + media.Close() + if b.HasSubscribers(TypeMprisUpdate) { + t.Fatal("closed subscriber still counted") + } + + all := b.Subscribe(4) + defer all.Close() + if !b.HasSubscribers(TypeMprisUpdate) { + t.Fatal("unfiltered subscriber must match every type") + } +} diff --git a/internal/plugins/mpris/mpris_test.go b/internal/plugins/mpris/mpris_test.go index 864a873..ca661e2 100644 --- a/internal/plugins/mpris/mpris_test.go +++ b/internal/plugins/mpris/mpris_test.go @@ -287,7 +287,12 @@ func TestStampAlbumArtMatchesCurrentTrack(t *testing.T) { } func TestPollRemoteStatesOnlyTargetsKnownPlayers(t *testing.T) { - plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) + bus := events.NewBus(log.Nop()) + // The poller only emits while somebody listens for mpris.update; + // subscribe so this test exercises the targeting logic itself. + watch := bus.Subscribe(4, events.TypeMprisUpdate) + defer watch.Close() + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) if plugin.watchCancel != nil { defer plugin.watchCancel() } diff --git a/internal/plugins/mpris/remote.go b/internal/plugins/mpris/remote.go index 8c3a940..b270eab 100644 --- a/internal/plugins/mpris/remote.go +++ b/internal/plugins/mpris/remote.go @@ -6,6 +6,7 @@ import ( "time" "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" "github.com/bethropolis/kcd/internal/protocol" ) @@ -192,11 +193,12 @@ func (p *MPRISPlugin) requestPlayerListPeriodic(dev device.Sender) { } // startRemoteStatePoller periodically re-requests now-playing from every -// connected device that has a known active player. The responses flow back -// through Handle, where shouldPublishRemoteState dedupes them, so an -// mpris.update is only republished when the state actually changes — not -// on every poll. This closes the "watch client misses mid-track state" -// gap from the initial dump's 10s freshness gate. +// connected device that has a known active player, but only while somebody +// listens: pollRemoteStates stays silent with zero mpris.update subscribers. +// The responses flow back through Handle, where shouldPublishRemoteState +// dedupes them, so an mpris.update is only republished when the state +// actually changes — not on every poll. This closes the "watch client +// misses mid-track state" gap from the initial dump's 10s freshness gate. func (p *MPRISPlugin) startRemoteStatePoller(ctx context.Context) { go func() { ticker := time.NewTicker(remoteStatePollInterval) @@ -217,7 +219,14 @@ func (p *MPRISPlugin) startRemoteStatePoller(ctx context.Context) { // reported a player) or whose player is stopped/paused are skipped — stopped // players are intentionally left to go stale instead of keeping a ghost track // perpetually fresh. +// +// The poller is demand-driven: with no subscriber for mpris.update (no +// `kcd watch` listening for media), answers would be consumed by nobody, so +// no requests go out. Subscribing re-arms the refresh within one interval. func (p *MPRISPlugin) pollRemoteStates() { + if !p.bus.HasSubscribers(events.TypeMprisUpdate) { + return + } p.mu.RLock() type target struct { dev device.Sender diff --git a/internal/plugins/mpris/remote_gate_test.go b/internal/plugins/mpris/remote_gate_test.go new file mode 100644 index 0000000..86c2965 --- /dev/null +++ b/internal/plugins/mpris/remote_gate_test.go @@ -0,0 +1,77 @@ +package mpris + +import ( + "testing" + + "github.com/bethropolis/kcd/internal/config" + "github.com/bethropolis/kcd/internal/events" + "github.com/bethropolis/kcd/internal/log" + "github.com/bethropolis/kcd/internal/protocol" +) + +type countingSender struct { + testSender + sends int +} + +func (s *countingSender) Send(_ *protocol.Packet) error { + s.sends++ + return nil +} + +func playingRemotePlugin(t *testing.T, bus *events.Bus) (*MPRISPlugin, *countingSender) { + t.Helper() + p := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) + sender := &countingSender{testSender: testSender{id: "dev1"}} + p.mu.Lock() + p.devices["dev1"] = sender + p.remoteStates["dev1"] = &NowPlaying{Player: "Spotify", IsPlaying: true} + p.mu.Unlock() + return p, sender +} + +// With nobody listening for mpris.update, the poller must send zero +// requests even while a remote player is playing. +func TestPollRemoteSilentWithoutSubscribers(t *testing.T) { + bus := events.NewBus(log.Nop()) + p, sender := playingRemotePlugin(t, bus) + + p.pollRemoteStates() + + if sender.sends != 0 { + t.Fatalf("sent %d requests with zero subscribers", sender.sends) + } +} + +// Subscribing for mpris.update re-arms the refresh: the next poll emits +// the status request for the playing remote. +func TestPollRemoteRefreshesWhileWatched(t *testing.T) { + bus := events.NewBus(log.Nop()) + p, sender := playingRemotePlugin(t, bus) + + sub := bus.Subscribe(4, events.TypeMprisUpdate) + defer sub.Close() + + p.pollRemoteStates() + + if sender.sends != 1 { + t.Fatalf("sent %d requests while watched, want 1", sender.sends) + } +} + +// Unsubscribing silences the poller again — attach/detach cycles must not +// leak refreshes. +func TestPollRemoteSilentAfterUnsubscribe(t *testing.T) { + bus := events.NewBus(log.Nop()) + p, sender := playingRemotePlugin(t, bus) + + sub := bus.Subscribe(4, events.TypeMprisUpdate) + p.pollRemoteStates() + sub.Close() + sender.sends = 0 + p.pollRemoteStates() + + if sender.sends != 0 { + t.Fatalf("sent %d requests after unsubscribe", sender.sends) + } +} From 68c64b5cf56c4d03c7e5f5e9a70fabfb1068365e Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 03:50:10 +0300 Subject: [PATCH 04/25] feat(reconnect): park redial on discovery sightings with stale horizon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reconnectWithBackoff no longer spins a per-device timer for usually-failing dials: a discovery sighting (peer provably alive) dials immediately via a wake channel, untriggered waits escalate to fallback_max (1h), and past stale_after (24h) of silence the loop exits entirely — zero timers for pairs that will never return. A future sighting respawns the loop from the discovery path with a fresh backoff. Mechanism: Device.reconnectWake (buffered-1, nil-safe), poked from the paired-sighting branch and on unpair transitions in SetState (no parked goroutine leak). Sighting bursts coalesce via a 5s trigger gap. sighting_driven=false restores the legacy pure-timer loop. New [reconnect] keys: sighting_driven (default true), fallback_max (default 1h, >= max_backoff), stale_after (default 24h). Existing configs overlay on defaults, so upgrades are unaffected. --- docs/CLI.md | 2 +- internal/config/config.go | 2 +- internal/config/config_test.go | 20 +- internal/config/runtime.go | 12 ++ internal/daemon/transport.go | 13 ++ internal/daemon/transport_reconnect.go | 59 +++++- internal/daemon/transport_reconnect_test.go | 194 ++++++++++++++++++++ internal/device/device_core.go | 39 +++- packaging/kcd.example.toml | 3 + 9 files changed, 335 insertions(+), 9 deletions(-) create mode 100644 internal/daemon/transport_reconnect_test.go diff --git a/docs/CLI.md b/docs/CLI.md index 7a7df8e..87aabe4 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -30,7 +30,7 @@ require a restart; reloading notification filters alone does not apply them. | Section | Settings and defaults | |---|---| | `[network]` | `dial_timeout = "5s"`, `handshake_timeout = "10s"`, `sidechannel_timeout = "15s"`, `transfer_idle_timeout = "60s"` | -| `[reconnect]` | `initial_backoff = "2s"`, `max_backoff = "5m"`, `flap_threshold = "15s"` | +| `[reconnect]` | `initial_backoff = "2s"`, `max_backoff = "5m"`, `flap_threshold = "15s"`, `sighting_driven = true`, `fallback_max = "1h"`, `stale_after = "24h"` | | `[discovery]` | `broadcast_interval = "30s"`, `broadcast_idle_interval = "60s"` | | `[mpris]` | `poll_while_playing = true`, `position_interval = "2s"` | | `[pairing]` | `intent_ttl = "5m"`, `listen_timeout = "60s"`; existing `timeout_secs = 30` still controls the pairing response wait | diff --git a/internal/config/config.go b/internal/config/config.go index ceef68f..14337a4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -70,7 +70,7 @@ func Defaults() *Config { c.LogLevel = "info" c.Network = NetworkConfig{DialTimeout: "5s", HandshakeTimeout: "10s", SidechannelTimeout: "15s", TransferIdleTimeout: "60s"} - c.Reconnect = ReconnectConfig{InitialBackoff: "2s", MaxBackoff: "5m", FlapThreshold: "15s"} + c.Reconnect = ReconnectConfig{InitialBackoff: "2s", MaxBackoff: "5m", FlapThreshold: "15s", SightingDriven: true, FallbackMax: "1h", StaleAfter: "24h"} c.Discovery = DiscoveryConfig{BroadcastInterval: "30s", BroadcastIdleInterval: "60s"} c.Plugins.Defaults() c.Commands = make(map[string]string) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 97050b1..191ba47 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -20,7 +20,7 @@ func TestDefaults(t *testing.T) { if cfg.Network != (NetworkConfig{"5s", "10s", "15s", "60s"}) { t.Errorf("network defaults: %+v", cfg.Network) } - if cfg.Reconnect != (ReconnectConfig{"2s", "5m", "15s"}) { + if cfg.Reconnect != (ReconnectConfig{"2s", "5m", "15s", true, "1h", "24h"}) { t.Errorf("reconnect defaults: %+v", cfg.Reconnect) } if cfg.Discovery != (DiscoveryConfig{"30s", "60s"}) { @@ -82,6 +82,9 @@ transfer_idle_timeout = "90s" initial_backoff = "3s" max_backoff = "6m" flap_threshold = "20s" +sighting_driven = false +fallback_max = "2h" +stale_after = "48h" [discovery] broadcast_interval = "45s" broadcast_idle_interval = "90s" @@ -102,7 +105,7 @@ app_name = "KDE Connect" if err != nil { t.Fatal(err) } - if cfg.Network != (NetworkConfig{"750ms", "12s", "25s", "90s"}) || cfg.Reconnect != (ReconnectConfig{"3s", "6m", "20s"}) || cfg.Discovery != (DiscoveryConfig{"45s", "90s"}) { + if cfg.Network != (NetworkConfig{"750ms", "12s", "25s", "90s"}) || cfg.Reconnect != (ReconnectConfig{"3s", "6m", "20s", false, "2h", "48h"}) || cfg.Discovery != (DiscoveryConfig{"45s", "90s"}) { t.Fatal("duration overrides not decoded") } if cfg.Cache != (CacheConfig{"/tmp/sms", "/tmp/art", "/tmp/contacts"}) || cfg.Pairing.IntentTTL != "7m" || cfg.Pairing.ListenTimeout != "2m" { @@ -132,6 +135,7 @@ func TestDurationValidation(t *testing.T) { fields := []struct{ section, key string }{ {"network", "dial_timeout"}, {"network", "handshake_timeout"}, {"network", "sidechannel_timeout"}, {"reconnect", "initial_backoff"}, {"reconnect", "max_backoff"}, {"reconnect", "flap_threshold"}, + {"reconnect", "fallback_max"}, {"reconnect", "stale_after"}, {"discovery", "broadcast_interval"}, {"discovery", "broadcast_idle_interval"}, {"pairing", "intent_ttl"}, {"pairing", "listen_timeout"}, {"mpris", "position_interval"}, @@ -167,6 +171,18 @@ func TestDurationRelationships(t *testing.T) { } } +func TestFallbackMaxRelationship(t *testing.T) { + // fallback_max must be >= max_backoff: the parked fallback may only + // space attempts wider than the legacy loop, never tighter. + if _, err := loadTOML(t, "[reconnect]\nfallback_max = '1m'\n"); err == nil || + !strings.Contains(err.Error(), "reconnect.fallback_max") { + t.Fatalf("expected fallback_max relationship error, got %v", err) + } + if _, err := loadTOML(t, "[reconnect]\nmax_backoff = '5m'\nfallback_max = '1h'\n"); err != nil { + t.Fatalf("wider fallback_max should be valid: %v", err) + } +} + func TestNotificationConfig(t *testing.T) { for _, cfg := range []NotificationConfig{nil, {}, {"app_name": ""}, {"*": "silent"}} { if cfg.AppName() != "KDE Connect" { diff --git a/internal/config/runtime.go b/internal/config/runtime.go index 1f0c955..5ad332f 100644 --- a/internal/config/runtime.go +++ b/internal/config/runtime.go @@ -20,6 +20,13 @@ type ReconnectConfig struct { InitialBackoff string `toml:"initial_backoff"` MaxBackoff string `toml:"max_backoff"` FlapThreshold string `toml:"flap_threshold"` + // SightingDriven parks the redial timer on discovery sightings: while + // set, the loop dials immediately on sighting and otherwise waits up + // to FallbackMax, giving up entirely past StaleAfter. False restores + // the legacy pure-timer loop capped at MaxBackoff. + SightingDriven bool `toml:"sighting_driven"` + FallbackMax string `toml:"fallback_max"` + StaleAfter string `toml:"stale_after"` } // DiscoveryConfig controls intervals while on-demand UDP discovery is running. @@ -61,6 +68,8 @@ func (c *Config) validateDurations() error { {"reconnect.initial_backoff", c.Reconnect.InitialBackoff}, {"reconnect.max_backoff", c.Reconnect.MaxBackoff}, {"reconnect.flap_threshold", c.Reconnect.FlapThreshold}, + {"reconnect.fallback_max", c.Reconnect.FallbackMax}, + {"reconnect.stale_after", c.Reconnect.StaleAfter}, {"discovery.broadcast_interval", c.Discovery.BroadcastInterval}, {"discovery.broadcast_idle_interval", c.Discovery.BroadcastIdleInterval}, {"pairing.intent_ttl", c.Pairing.IntentTTL}, @@ -78,6 +87,9 @@ func (c *Config) validateDurations() error { if Duration(c.Reconnect.MaxBackoff) < Duration(c.Reconnect.InitialBackoff) { return fmt.Errorf("config: reconnect.max_backoff must be >= reconnect.initial_backoff") } + if Duration(c.Reconnect.FallbackMax) < Duration(c.Reconnect.MaxBackoff) { + return fmt.Errorf("config: reconnect.fallback_max must be >= reconnect.max_backoff") + } if Duration(c.Discovery.BroadcastIdleInterval) < Duration(c.Discovery.BroadcastInterval) { return fmt.Errorf("config: discovery.broadcast_idle_interval must be >= discovery.broadcast_interval") } diff --git a/internal/daemon/transport.go b/internal/daemon/transport.go index 5cb25e4..9c15f94 100644 --- a/internal/daemon/transport.go +++ b/internal/daemon/transport.go @@ -180,6 +180,19 @@ func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.Broadcaste } } } + if opts.Reconnect.SightingDriven { + // Wake a parked reconnect loop (peer provably alive at + // this address), or respawn one that gave up past the + // stale horizon. TryReconnect single-flights: exactly one + // loop per device. The sighted address replaces a stale + // LastIP, and the attempt counter restarts — the peer is + // provably back, so escalated backoff no longer applies. + dev.PokeReconnect() + if !dev.IsConnected() && dev.TryReconnect() { + dev.ResetReconnectAttempt() + go reconnectWithBackoff(ctx, dev, ip, identity, cfg, devices, plugins, localDeviceID, logger, opts) + } + } return } diff --git a/internal/daemon/transport_reconnect.go b/internal/daemon/transport_reconnect.go index c1751ec..5aaf8ee 100644 --- a/internal/daemon/transport_reconnect.go +++ b/internal/daemon/transport_reconnect.go @@ -13,12 +13,24 @@ import ( "github.com/bethropolis/kcd/internal/protocol" ) +// reconnectTriggerMinGap is the minimum spacing between sighting-triggered +// dials. Announcements can arrive in bursts (IPv4/IPv6 alternation, AP +// flicker); the gap keeps a burst to a single dial while the fallback +// timer keeps spacing untriggered attempts. +const reconnectTriggerMinGap = 5 * time.Second + // reconnectWithBackoff dials a paired device after it disconnects, using -// exponential backoff up to 5 minutes between attempts. It stops as soon as: +// exponential backoff between attempts. It stops as soon as: // - the device reconnects (IsConnected becomes true), or // - the daemon context is cancelled, or // - the device is unpaired. // +// With sighting-driven mode (reconnect.sighting_driven, the default) the +// loop parks instead of spinning a timer per backoff step: a discovery +// sighting (the peer provably alive at an address) dials immediately, and +// the fallback timer escalates to fallback_max (default 1h) for the silent +// case. Past stale_after (default 24h) without any sighting the loop gives +// up entirely — a future sighting respawns it from the discovery path. // A fresh connection coming in from the phone side (inbound TCP) will set // IsConnected, causing the loop to exit cleanly without a duplicate dial. func reconnectWithBackoff( @@ -34,7 +46,17 @@ func reconnectWithBackoff( opts *config.Config, ) { maxBackoff := config.Duration(opts.Reconnect.MaxBackoff) + waitCap := maxBackoff + sightingDriven := opts.Reconnect.SightingDriven + staleAfter := time.Duration(0) + if sightingDriven { + waitCap = config.Duration(opts.Reconnect.FallbackMax) + staleAfter = config.Duration(opts.Reconnect.StaleAfter) + } attempt := dev.ReconnectAttempt() + // Zero until the first dial: the trigger gap guards between dials, + // and loop start is not a dial — the first sighting always dials. + var lastDial time.Time defer dev.ReconnectDone() @@ -42,6 +64,7 @@ func reconnectWithBackoff( log.String("device_id", dev.ID()), log.String("device_name", dev.Name()), log.String("ip", ip.String()), + log.Bool("sighting_driven", sightingDriven), ) for { @@ -64,28 +87,55 @@ func reconnectWithBackoff( return } - backoff := device.ReconnectBackoff(attempt, maxBackoff, config.Duration(opts.Reconnect.InitialBackoff)) + // Stop if the peer has been silent past the horizon: no sighting + // for a day means it is gone, not roaming. Zero timers until a + // future sighting respawns this loop from the discovery path. + if sightingDriven && time.Since(dev.LastSeen()) > staleAfter { + logger.Info("auto-reconnect: device stale, giving up until next sighting", + log.String("device_id", dev.ID()), + log.Duration("stale_after", staleAfter), + ) + return + } + + backoff := device.ReconnectBackoff(attempt, waitCap, config.Duration(opts.Reconnect.InitialBackoff)) logger.Debug("auto-reconnect: waiting before next attempt", log.String("device_id", dev.ID()), log.Int("attempt", attempt+1), log.Duration("backoff", backoff), ) + timer := time.NewTimer(backoff) + triggered := false select { case <-ctx.Done(): + timer.Stop() return - case <-time.After(backoff): + case <-dev.ReconnectWake(): + // Sighting (peer alive) or unpair. Stop the timer and + // re-check below; a fresh sighting dials immediately. + timer.Stop() + triggered = true + case <-timer.C: } - // Re-check after the sleep — the phone may have connected inbound. + // Re-check after the wait — the phone may have connected inbound, + // or been unpaired while parked. if dev.IsConnected() || dev.State() != device.StatePaired { return } + if triggered && time.Since(lastDial) < reconnectTriggerMinGap { + // Sighting burst (dual-stack/AP flicker): skip this dial, + // keep parking. The backoff is recomputed next lap. + continue + } + logger.Info("auto-reconnect: dialling", log.String("device_id", dev.ID()), log.String("ip", ip.String()), log.Int("attempt", attempt+1), + log.Bool("sighting_triggered", triggered), ) // Prefer the peer's last advertised listening port over the @@ -93,6 +143,7 @@ func reconnectWithBackoff( // at all, in which case LastPort is 0 and we fall back). port := reconnectPort(dev, opts.TCPPort) DialDevice(ctx, ip, port, dev.ID(), protocol.ProtocolVersion, identity, cfg, devices, plugins, localDeviceID, logger, false, opts) + lastDial = time.Now() if dev.IsConnected() { logger.Info("auto-reconnect: succeeded", diff --git a/internal/daemon/transport_reconnect_test.go b/internal/daemon/transport_reconnect_test.go new file mode 100644 index 0000000..e39cdda --- /dev/null +++ b/internal/daemon/transport_reconnect_test.go @@ -0,0 +1,194 @@ +package daemon + +import ( + "context" + "crypto/tls" + "net" + "sync/atomic" + "testing" + "time" + + "github.com/bethropolis/kcd/internal/config" + "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/log" + "github.com/bethropolis/kcd/internal/plugin" +) + +// parkedOpts returns reconnect options whose timer never fires during the +// test, so any dial must come from an explicit sighting poke. +func parkedOpts() *config.Config { + opts := config.Defaults() + opts.Reconnect.SightingDriven = true + opts.Reconnect.InitialBackoff = "1h" + opts.Reconnect.MaxBackoff = "5m" + opts.Reconnect.FallbackMax = "1h" + opts.Reconnect.StaleAfter = "24h" + return opts +} + +func parkedDevice(t *testing.T, id string) *device.Device { + t.Helper() + dev := device.NewDevice(id, "Phone", "phone", log.Nop()) + dev.SetState(device.StatePaired) + dev.SetLastSeen(time.Now()) + if !dev.TryReconnect() { + t.Fatal("TryReconnect must succeed for a fresh device") + } + return dev +} + +// acceptCounter listens on loopback and counts inbound dials, closing each +// immediately so the dial side fails fast. +type acceptCounter struct { + ln net.Listener + count atomic.Int32 +} + +func newAcceptCounter(t *testing.T) *acceptCounter { + t.Helper() + ln, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + ac := &acceptCounter{ln: ln} + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + ac.count.Add(1) + c.Close() + } + }() + t.Cleanup(func() { ln.Close() }) + return ac +} + +func (ac *acceptCounter) port() int { + return ac.ln.Addr().(*net.TCPAddr).Port +} + +func waitForReconnectExit(t *testing.T, dev *device.Device, timeout time.Duration) { + t.Helper() + deadline := time.Now().Add(timeout) + for dev.Reconnecting() && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if dev.Reconnecting() { + t.Fatalf("reconnect loop still running after %v", timeout) + } +} + +// A device silent past the stale horizon must exit without dialling: +// zero timers for pairs that will never return. +func TestReconnectStaleHorizonGivesUp(t *testing.T) { + ac := newAcceptCounter(t) + dev := parkedDevice(t, "stale-1") + dev.SetLastSeen(time.Now().Add(-25 * time.Hour)) + dev.SetLastPort(ac.port()) + + opts := parkedOpts() + identity, err := newTestIdentity() + if err != nil { + t.Fatal(err) + } + go reconnectWithBackoff(context.Background(), dev, net.ParseIP("127.0.0.1"), identity, + &tls.Config{InsecureSkipVerify: true}, device.NewRegistry(nil), + plugin.NewRegistry(log.Nop()), "local", log.Nop(), opts) + + waitForReconnectExit(t, dev, 5*time.Second) + if n := ac.count.Load(); n != 0 { + t.Fatalf("stale device dialled %d times, want 0", n) + } +} + +// A sighting poke must dial immediately even with a 1h fallback timer, +// and a burst of pokes must coalesce to a single dial (min-gap guard). +func TestReconnectWakesOnSighting(t *testing.T) { + ac := newAcceptCounter(t) + dev := parkedDevice(t, "roam-1") + dev.SetLastPort(ac.port()) + + opts := parkedOpts() + identity, err := newTestIdentity() + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go reconnectWithBackoff(ctx, dev, net.ParseIP("127.0.0.1"), identity, + &tls.Config{InsecureSkipVerify: true}, device.NewRegistry(nil), + plugin.NewRegistry(log.Nop()), "local", log.Nop(), opts) + + time.Sleep(100 * time.Millisecond) // let the loop park on the timer + dev.PokeReconnect() + dev.PokeReconnect() + dev.PokeReconnect() + + deadline := time.Now().Add(5 * time.Second) + for ac.count.Load() == 0 && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if ac.count.Load() == 0 { + t.Fatal("sighting poke produced no dial within 5s") + } + time.Sleep(400 * time.Millisecond) + if n := ac.count.Load(); n != 1 { + t.Fatalf("sighting burst produced %d dials, want exactly 1", n) + } +} + +// Unpairing while parked must exit the loop promptly (no goroutine leak). +func TestReconnectUnpairExitsParkedLoop(t *testing.T) { + dev := parkedDevice(t, "gone-1") + + opts := parkedOpts() + identity, err := newTestIdentity() + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go reconnectWithBackoff(ctx, dev, net.ParseIP("192.0.2.1"), identity, + &tls.Config{InsecureSkipVerify: true}, device.NewRegistry(nil), + plugin.NewRegistry(log.Nop()), "local", log.Nop(), opts) + + time.Sleep(100 * time.Millisecond) // let the loop park + dev.SetState(device.StateUnpaired) // pokes the wake channel + + waitForReconnectExit(t, dev, 5*time.Second) +} + +// Legacy mode (sighting_driven=false) keeps pure-timer dials capped at +// max_backoff: the timer path must still fire without any sighting. +func TestReconnectLegacyTimerStillDials(t *testing.T) { + ac := newAcceptCounter(t) + dev := parkedDevice(t, "legacy-1") + dev.SetLastPort(ac.port()) + dev.SetLastSeen(time.Now()) + + opts := config.Defaults() + opts.Reconnect.SightingDriven = false + opts.Reconnect.InitialBackoff = "20ms" + opts.Reconnect.MaxBackoff = "20ms" + opts.Reconnect.FlapThreshold = "15s" + + identity, err := newTestIdentity() + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go reconnectWithBackoff(ctx, dev, net.ParseIP("127.0.0.1"), identity, + &tls.Config{InsecureSkipVerify: true}, device.NewRegistry(nil), + plugin.NewRegistry(log.Nop()), "local", log.Nop(), opts) + + deadline := time.Now().Add(5 * time.Second) + for ac.count.Load() == 0 && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if ac.count.Load() == 0 { + t.Fatal("legacy timer produced no dial within 5s") + } +} diff --git a/internal/device/device_core.go b/internal/device/device_core.go index 4963445..a4a9bc9 100644 --- a/internal/device/device_core.go +++ b/internal/device/device_core.go @@ -97,6 +97,12 @@ type Device struct { // auto-reconnect goroutines for this device. reconnecting atomic.Bool + // reconnectWake nudges a parked auto-reconnect loop: discovery + // sightings (peer provably alive) and unpair transitions. Buffered-1 + // so pokes never block the discovery listener; coalesced bursts mean + // "check now", not N dials. + reconnectWake chan struct{} + // reconnectAttempt persists the auto-reconnect backoff counter across // disconnect cycles. A connection that flaps (drops shortly after a // successful dial) keeps the counter so the backoff escalates instead of @@ -127,7 +133,11 @@ func NewDevice(id, name, dtype string, logger log.Logger) *Device { state: StateUnpaired, sendChan: make(chan *protocol.Packet, 32), done: make(chan struct{}), - logger: logger.With(log.String("device_id", id)), + // Nil-safe by construction, but PokeReconnect also tolerates a + // zero-value Device (tests): a send on a nil channel blocks, so + // the select always takes the default branch. + reconnectWake: make(chan struct{}, 1), + logger: logger.With(log.String("device_id", id)), } } @@ -160,6 +170,33 @@ func (d *Device) SetState(s PairingState) { d.mu.Lock() defer d.mu.Unlock() d.state = s + // Wake a parked reconnect loop so unpair takes effect immediately + // instead of at the next timer fire (or never, past the stale + // horizon). The loop re-checks state on wake and exits. + if s == StateUnpaired { + d.PokeReconnect() + } +} + +// PokeReconnect nudges the auto-reconnect loop to re-check now (sighting +// arrived, or state changed). Non-blocking and nil-safe: bursts coalesce +// into a single wakeup. +func (d *Device) PokeReconnect() { + select { + case d.reconnectWake <- struct{}{}: + default: + } +} + +// ReconnectWake exposes the wake channel for the auto-reconnect loop's +// select. A nil channel (zero-value Device) blocks forever — safe. +func (d *Device) ReconnectWake() <-chan struct{} { + return d.reconnectWake +} + +// Reconnecting reports whether an auto-reconnect goroutine is running. +func (d *Device) Reconnecting() bool { + return d.reconnecting.Load() } func (d *Device) LastSeen() time.Time { d.mu.RLock() diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index 33e4d5b..278fc5b 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -177,6 +177,9 @@ remotesystemvolume = true # initial_backoff = "2s" # max_backoff = "5m" # must be >= initial_backoff # flap_threshold = "15s" # minimum lifetime for a stable connection +# sighting_driven = true # park redial on discovery sightings; false = legacy pure-timer loop +# fallback_max = "1h" # must be >= max_backoff; silent-case spacing while parked +# stale_after = "24h" # give up (zero timers) past this silence; next sighting respawns [discovery] # broadcast_interval = "30s" From f260062434d48bfd44016bf14230a86afb4c361e Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 04:06:14 +0300 Subject: [PATCH 05/25] feat(discovery): run mDNS browse under broadcast ownership zeroconf Browse re-queries periodically (4s exponential backoff to 60s cap) plus a 10s cache-cleanup ticker, so lifetime-on browsing is a fourth standing wakeup source. Browse now shares the broadcast controller lifetime: it runs while pairing or reconnect owners hold the loop and stops at connected steady state, where the lifetime UDP listener and (responder-only) mDNS advertisement cover inbound discovery. Wiring: BroadcasterController.SetBrowseStarter, launched on the same owned context in StartOwned; daemon registers Listener.RunMdnsDiscovery in runTransport. Also closes the entries channel when Browse returns, fixing a leaked results goroutine on every browse stop. --- docs/ARCHITECTURE.md | 4 +- internal/daemon/transport.go | 6 +++ internal/discovery/broadcaster_controller.go | 15 +++++++ internal/discovery/discovery_test.go | 44 ++++++++++++++++++++ internal/discovery/listener.go | 5 +-- internal/discovery/listener_mdns.go | 12 +++++- 6 files changed, 79 insertions(+), 7 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 22339f8..e3b839a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -75,7 +75,7 @@ with no pair in flight are disconnected immediately. At startup the `Broadcaster` registers the local device as a Zeroconf service with the `libp2p/zeroconf/v2` library. TXT records carry `id`, `name`, `type`, and `protocol` fields per the KDE Connect spec. -`Listener.runMdnsDiscovery` browses `_kdeconnect._udp.local.` and synthesises a `protocol.Packet` for every discovered peer — feeding it through the same `onDeviceFound` callback used by UDP. This makes mDNS transparent to the rest of the stack. +`Listener.RunMdnsDiscovery` browses `_kdeconnect._udp.local.` and synthesises a `protocol.Packet` for every discovered peer — feeding it through the same `onDeviceFound` callback used by UDP. This makes mDNS transparent to the rest of the stack. Browsing shares the broadcast ownership lifetime (pairing/reconnect only): the library re-queries periodically, so lifetime-on browsing would cost standing timers at connected steady state, where the always-on UDP listener and advertisement cover inbound discovery. **Why both?** UDP broadcast covers the common case instantly. mDNS handles restricted networks (Docker bridges, enterprise Wi-Fi, newer Android versions) where broadcast is filtered. @@ -357,7 +357,7 @@ kcd watch [--events=...] [--json] — stream live events 4. Register all enabled plugins 5. Start the TCP listener (inbound connections) 6. Start the IPC Unix socket server -7. Start the UDP listener and mDNS browser (discovery listener — always on) +7. Start the UDP listener (always on) and register mDNS browsing with the broadcast controller (active only while pairing or reconnect owners hold it) 8. Create the `BroadcasterController` in stopped state (broadcast is off by default) 9. Block until context is cancelled (SIGINT / SIGTERM) 10. Graceful shutdown: close listener, shutdown mDNS, stop broadcaster (if running) diff --git a/internal/daemon/transport.go b/internal/daemon/transport.go index 9c15f94..448d7ba 100644 --- a/internal/daemon/transport.go +++ b/internal/daemon/transport.go @@ -230,6 +230,12 @@ func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.Broadcaste } udpListener := discovery.NewListener(opts.TCPPort, localDeviceID, onDeviceFound, logger) + // Active discovery shares the broadcast ownership lifetime: mDNS + // browsing (periodic probes) runs only while pairing or reconnect + // owners hold the controller, never at connected steady state. + if bc != nil { + bc.SetBrowseStarter(udpListener.RunMdnsDiscovery) + } go udpListener.Run(ctx) // Accept loop diff --git a/internal/discovery/broadcaster_controller.go b/internal/discovery/broadcaster_controller.go index e9131cf..8c8d68a 100644 --- a/internal/discovery/broadcaster_controller.go +++ b/internal/discovery/broadcaster_controller.go @@ -31,6 +31,10 @@ type BroadcasterController struct { running bool cancel context.CancelFunc owners map[string]struct{} + // browseStarter, when set, launches mDNS browsing on the same owned + // context as the broadcast loop: active discovery (both directions) + // shares one lifetime — pairing/reconnect only, never steady state. + browseStarter func(ctx context.Context) } // defaultIdleInterval is the broadcast period while idle (all pairs @@ -68,6 +72,14 @@ func (bc *BroadcasterController) Stop() { bc.StopOwned(OwnerPairing) } +// SetBrowseStarter registers the mDNS browse function to run alongside +// the owned broadcast loop. Called once at startup before any StartOwned. +func (bc *BroadcasterController) SetBrowseStarter(starter func(ctx context.Context)) { + bc.mu.Lock() + defer bc.mu.Unlock() + bc.browseStarter = starter +} + // StartOwned launches the loop (if needed) and records owner as needing it. func (bc *BroadcasterController) StartOwned(parentCtx context.Context, owner string) { bc.mu.Lock() @@ -95,6 +107,9 @@ func (bc *BroadcasterController) StartOwned(parentCtx context.Context, owner str bc.running = false bc.mu.Unlock() }() + if bc.browseStarter != nil { + go bc.browseStarter(ctx) + } } // StopOwned withdraws owner's need. No-op if the owner holds nothing. diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go index 74cc29f..6f9787d 100644 --- a/internal/discovery/discovery_test.go +++ b/internal/discovery/discovery_test.go @@ -61,6 +61,50 @@ func TestBroadcasterOwners(t *testing.T) { } } +// The mDNS browse starter shares the owned loop lifetime: it launches on +// StartOwned and its context ends on the last StopOwned, so periodic +// browse probes never run at steady state. +func TestBrowseStarterSharesOwnership(t *testing.T) { + bc := NewBroadcasterController(testIdentity(t), protocol.DefaultTCPPort, time.Hour, log.Nop(), nil) + ctx := context.Background() + + started := make(chan struct{}) + stopped := make(chan struct{}) + bc.SetBrowseStarter(func(browseCtx context.Context) { + close(started) + <-browseCtx.Done() + close(stopped) + }) + + bc.StartOwned(ctx, OwnerReconnect) + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("browse starter not launched with owned loop") + } + + bc.StopOwned(OwnerReconnect) + select { + case <-stopped: + case <-time.After(2 * time.Second): + t.Fatal("browse context not cancelled with owned loop") + } +} + +// Without a registered starter the controller behaves exactly as before. +func TestNoBrowseStarterNoBrowse(t *testing.T) { + bc := NewBroadcasterController(testIdentity(t), protocol.DefaultTCPPort, time.Hour, log.Nop(), nil) + ctx := context.Background() + bc.StartOwned(ctx, OwnerReconnect) + if !bc.IsRunning() { + t.Fatal("loop must run without a browse starter") + } + bc.StopOwned(OwnerReconnect) + if bc.IsRunning() { + t.Fatal("loop must stop without a browse starter") + } +} + // A non-default tcp_port must reach the broadcaster: the controller stores // it and hands it to every Broadcaster it spawns. func TestBroadcasterControllerStoresPort(t *testing.T) { diff --git a/internal/discovery/listener.go b/internal/discovery/listener.go index 55fbec6..91f7ce5 100644 --- a/internal/discovery/listener.go +++ b/internal/discovery/listener.go @@ -28,10 +28,9 @@ func NewListener(port int, localDeviceID string, callback func(ip net.IP, tcpPor } // Run starts the UDP listener loop to parse incoming discovery broadcasts. +// Lifetime-on: blocking read, zero timers. mDNS browsing is owned +// separately by the broadcast controller (see RunMdnsDiscovery). func (l *Listener) Run(ctx context.Context) { - // mDNS Discovery - go l.runMdnsDiscovery(ctx) - addr := &net.UDPAddr{Port: l.port} conn, err := net.ListenUDP("udp", addr) if err != nil { diff --git a/internal/discovery/listener_mdns.go b/internal/discovery/listener_mdns.go index de1f224..96ee105 100644 --- a/internal/discovery/listener_mdns.go +++ b/internal/discovery/listener_mdns.go @@ -46,12 +46,20 @@ func AdvertiseMDNS(ctx context.Context, identityPacket *protocol.Packet, logger }() } -// runMdnsDiscovery browses for peer _kdeconnect._udp services and feeds +// RunMdnsDiscovery browses for peer _kdeconnect._udp services and feeds // sightings to onDeviceFound as synthetic identity packets. It is a method // on Listener (kept apart from the UDP Run loop) so both transports share // the same callback and self-filtering. -func (l *Listener) runMdnsDiscovery(ctx context.Context) { +// +// It runs only while the broadcast controller holds owners (pairing or +// reconnect): zeroconf Browse re-queries periodically (4s backoff to 60s) +// plus a 10s cache-cleanup ticker, which is incompatible with zero-idle +// steady state. Connected steady state relies on the lifetime UDP listener +// and mDNS advertisement for inbound discovery instead. Returns when ctx +// ends; the entries channel is closed so the results loop exits too. +func (l *Listener) RunMdnsDiscovery(ctx context.Context) { entries := make(chan *zeroconf.ServiceEntry) + defer close(entries) go func(results <-chan *zeroconf.ServiceEntry) { for entry := range results { var deviceId, deviceName, deviceType string From 586b7069835fb10c9fe26d86da54a704d4c18e9e Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 04:12:46 +0300 Subject: [PATCH 06/25] feat(network): keepalive_idle knob + idle-behavior docs [network] keepalive_idle (default 30s, floor 10s) tunes the TCP keepalive first-probe delay on the inbound listener, outbound dials, and side-channel dials, which previously shared a hardcoded 30s. Behavior at default is byte-identical; larger values trade slower zombie detection for fewer kernel probes on idle connections. Also documents the release invariant in ARCHITECTURE.md: connected steady state keeps zero application timers, with the gating table for every former periodic source plus measured numbers (2 ticks/min, 0 D-Bus calls/min, down from 13 + 30). New periodic work must be owner- or activity-gated, never standing. --- docs/ARCHITECTURE.md | 21 +++++++++++++++++++++ docs/CLI.md | 2 +- internal/config/config.go | 2 +- internal/config/config_test.go | 17 +++++++++++++++-- internal/config/runtime.go | 10 ++++++++++ internal/daemon/plugins.go | 5 +++-- internal/daemon/transport.go | 1 + internal/daemon/transport_dial.go | 2 +- internal/transport/keepalive.go | 28 ++++++++++++++++------------ internal/transport/listener.go | 12 +++++++++++- internal/transport/sidechannel.go | 8 +++++++- packaging/kcd.example.toml | 2 ++ 12 files changed, 89 insertions(+), 21 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e3b839a..635b5fd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -81,6 +81,27 @@ At startup the `Broadcaster` registers the local device as a Zeroconf service wi --- +## Idle behavior (zero standing timers) + +Connected steady state (all pairs connected, nothing playing, no transfers, no pairing) keeps **zero application timers**. Every periodic source is gated on actual activity instead of running free: + +| Source | Gating | +|---|---| +| UDP broadcast | Owned: pairing + reconnect owners only; zero owners = zero timers | +| mDNS browse | Same owned lifetime as broadcast (probes are periodic by library design) | +| mDNS advertise | Lifetime-on, responder-only (no timers) | +| UDP/TCP/IPC listeners, D-Bus signals, bus subscriptions | Blocking waits, zero CPU until an event arrives | +| Local position poller | Exists only while ≥1 local player `IsPlaying` (`[mpris] poll_while_playing`, `position_interval`) | +| Remote state poller | Fires only while a client subscribes to `mpris.update` | +| Reconnect redial | Parked on discovery sightings; fallback escalates to `fallback_max`, then gives up past `stale_after` until the next sighting | +| TCP keepalive | Kernel probes, first delay `[network] keepalive_idle` (default 30s, minimum 10s) | + +Measured 2026-09-22 (phone connected, zero local players, one watch subscriber): 2 CPU ticks/min, 0 D-Bus `GetAll`/min — down from 13 ticks/min + 30 `GetAll`/min before. Methodology note: Go timers are runtime-managed (no timerfds to count) and `ptrace` is restricted by Yama, so `/proc` CPU deltas + `dbus-monitor` call rates are the working proxies. + +**Contributor invariant: new periodic work must be owner-gated or activity-gated, never standing.** A ticker that fires while nothing is happening is a bug — gate it on owners (discovery), playback state (MPRIS), subscribers (remote refresh), or sightings (reconnect). + +--- + ## 2. Transport (`internal/transport`) The KDE Connect TLS handshake is non-standard: **the TCP initiator acts as TLS server, and the acceptor acts as TLS client**. This is the opposite of conventional TLS and must be handled correctly. diff --git a/docs/CLI.md b/docs/CLI.md index 87aabe4..3ddf867 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -29,7 +29,7 @@ require a restart; reloading notification filters alone does not apply them. | Section | Settings and defaults | |---|---| -| `[network]` | `dial_timeout = "5s"`, `handshake_timeout = "10s"`, `sidechannel_timeout = "15s"`, `transfer_idle_timeout = "60s"` | +| `[network]` | `dial_timeout = "5s"`, `handshake_timeout = "10s"`, `sidechannel_timeout = "15s"`, `transfer_idle_timeout = "60s"`, `keepalive_idle = "30s"` (minimum `"10s"`) | | `[reconnect]` | `initial_backoff = "2s"`, `max_backoff = "5m"`, `flap_threshold = "15s"`, `sighting_driven = true`, `fallback_max = "1h"`, `stale_after = "24h"` | | `[discovery]` | `broadcast_interval = "30s"`, `broadcast_idle_interval = "60s"` | | `[mpris]` | `poll_while_playing = true`, `position_interval = "2s"` | diff --git a/internal/config/config.go b/internal/config/config.go index 14337a4..03df368 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -69,7 +69,7 @@ func Defaults() *Config { c.TCPPort = protocol.DefaultTCPPort c.LogLevel = "info" - c.Network = NetworkConfig{DialTimeout: "5s", HandshakeTimeout: "10s", SidechannelTimeout: "15s", TransferIdleTimeout: "60s"} + c.Network = NetworkConfig{DialTimeout: "5s", HandshakeTimeout: "10s", SidechannelTimeout: "15s", TransferIdleTimeout: "60s", KeepAliveIdle: "30s"} c.Reconnect = ReconnectConfig{InitialBackoff: "2s", MaxBackoff: "5m", FlapThreshold: "15s", SightingDriven: true, FallbackMax: "1h", StaleAfter: "24h"} c.Discovery = DiscoveryConfig{BroadcastInterval: "30s", BroadcastIdleInterval: "60s"} c.Plugins.Defaults() diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 191ba47..0405ed6 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -17,7 +17,7 @@ func TestDefaults(t *testing.T) { if err := cfg.Validate(); err != nil { t.Fatal(err) } - if cfg.Network != (NetworkConfig{"5s", "10s", "15s", "60s"}) { + if cfg.Network != (NetworkConfig{"5s", "10s", "15s", "60s", "30s"}) { t.Errorf("network defaults: %+v", cfg.Network) } if cfg.Reconnect != (ReconnectConfig{"2s", "5m", "15s", true, "1h", "24h"}) { @@ -105,7 +105,7 @@ app_name = "KDE Connect" if err != nil { t.Fatal(err) } - if cfg.Network != (NetworkConfig{"750ms", "12s", "25s", "90s"}) || cfg.Reconnect != (ReconnectConfig{"3s", "6m", "20s", false, "2h", "48h"}) || cfg.Discovery != (DiscoveryConfig{"45s", "90s"}) { + if cfg.Network != (NetworkConfig{"750ms", "12s", "25s", "90s", "30s"}) || cfg.Reconnect != (ReconnectConfig{"3s", "6m", "20s", false, "2h", "48h"}) || cfg.Discovery != (DiscoveryConfig{"45s", "90s"}) { t.Fatal("duration overrides not decoded") } if cfg.Cache != (CacheConfig{"/tmp/sms", "/tmp/art", "/tmp/contacts"}) || cfg.Pairing.IntentTTL != "7m" || cfg.Pairing.ListenTimeout != "2m" { @@ -134,6 +134,7 @@ app_name = "KDE Connect" func TestDurationValidation(t *testing.T) { fields := []struct{ section, key string }{ {"network", "dial_timeout"}, {"network", "handshake_timeout"}, {"network", "sidechannel_timeout"}, + {"network", "keepalive_idle"}, {"reconnect", "initial_backoff"}, {"reconnect", "max_backoff"}, {"reconnect", "flap_threshold"}, {"reconnect", "fallback_max"}, {"reconnect", "stale_after"}, {"discovery", "broadcast_interval"}, {"discovery", "broadcast_idle_interval"}, @@ -183,6 +184,18 @@ func TestFallbackMaxRelationship(t *testing.T) { } } +func TestKeepAliveIdleFloor(t *testing.T) { + // Below 10s the radio never sleeps: reject, even though the value + // parses as a positive duration. + if _, err := loadTOML(t, "[network]\nkeepalive_idle = '5s'\n"); err == nil || + !strings.Contains(err.Error(), "network.keepalive_idle") { + t.Fatalf("expected keepalive_idle floor error, got %v", err) + } + if _, err := loadTOML(t, "[network]\nkeepalive_idle = '120s'\n"); err != nil { + t.Fatalf("120s keepalive_idle should be valid: %v", err) + } +} + func TestNotificationConfig(t *testing.T) { for _, cfg := range []NotificationConfig{nil, {}, {"app_name": ""}, {"*": "silent"}} { if cfg.AppName() != "KDE Connect" { diff --git a/internal/config/runtime.go b/internal/config/runtime.go index 5ad332f..9eccb91 100644 --- a/internal/config/runtime.go +++ b/internal/config/runtime.go @@ -8,11 +8,17 @@ import ( // NetworkConfig controls connection setup deadlines, not transfer size limits. // TransferIdleTimeout bounds streaming silence on side-channel transfers: // any read/write gap longer than it aborts the transfer. +// KeepAliveIdle is the TCP keepalive first-probe delay (kernel timers, not +// application wakeups): larger values send fewer probes on idle +// connections but detect dead peers more slowly. Never zero — idle +// sockets without probes become undetectable zombies (the per-write +// deadline guards writers only, not idle sockets). type NetworkConfig struct { DialTimeout string `toml:"dial_timeout"` HandshakeTimeout string `toml:"handshake_timeout"` SidechannelTimeout string `toml:"sidechannel_timeout"` TransferIdleTimeout string `toml:"transfer_idle_timeout"` + KeepAliveIdle string `toml:"keepalive_idle"` } // ReconnectConfig controls retry delays and the minimum stable connection age. @@ -65,6 +71,7 @@ func (c *Config) validateDurations() error { {"network.handshake_timeout", c.Network.HandshakeTimeout}, {"network.sidechannel_timeout", c.Network.SidechannelTimeout}, {"network.transfer_idle_timeout", c.Network.TransferIdleTimeout}, + {"network.keepalive_idle", c.Network.KeepAliveIdle}, {"reconnect.initial_backoff", c.Reconnect.InitialBackoff}, {"reconnect.max_backoff", c.Reconnect.MaxBackoff}, {"reconnect.flap_threshold", c.Reconnect.FlapThreshold}, @@ -90,6 +97,9 @@ func (c *Config) validateDurations() error { if Duration(c.Reconnect.FallbackMax) < Duration(c.Reconnect.MaxBackoff) { return fmt.Errorf("config: reconnect.fallback_max must be >= reconnect.max_backoff") } + if Duration(c.Network.KeepAliveIdle) < 10*time.Second { + return fmt.Errorf("config: network.keepalive_idle must be >= 10s (shorter delays keep weak radios awake)") + } if Duration(c.Discovery.BroadcastIdleInterval) < Duration(c.Discovery.BroadcastInterval) { return fmt.Errorf("config: discovery.broadcast_idle_interval must be >= discovery.broadcast_interval") } diff --git a/internal/daemon/plugins.go b/internal/daemon/plugins.go index 7d4f6be..b7c161a 100644 --- a/internal/daemon/plugins.go +++ b/internal/daemon/plugins.go @@ -34,8 +34,9 @@ import ( func setupPlugins(cfg *config.Config, bus *events.Bus, tlsCfg *tls.Config, logger log.Logger, devices *device.Registry, localCert *x509.Certificate, saveDevices func(), plugins *plugin.Registry) *pair.PairPlugin { sidechannel := transport.SidechannelOptions{ - Timeout: config.Duration(cfg.Network.SidechannelTimeout), - IdleTimeout: config.Duration(cfg.Network.TransferIdleTimeout), + Timeout: config.Duration(cfg.Network.SidechannelTimeout), + IdleTimeout: config.Duration(cfg.Network.TransferIdleTimeout), + KeepAliveIdle: config.Duration(cfg.Network.KeepAliveIdle), } pairPlugin := pair.NewPairPlugin(devices, localCert, cfg.Pairing, saveDevices, bus, logger) plugins.Register(pairPlugin) diff --git a/internal/daemon/transport.go b/internal/daemon/transport.go index 448d7ba..95e2b29 100644 --- a/internal/daemon/transport.go +++ b/internal/daemon/transport.go @@ -55,6 +55,7 @@ func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.Broadcaste return } defer tcpListener.Close() + tcpListener.SetKeepAliveIdle(config.Duration(opts.Network.KeepAliveIdle)) // Broadcast is off by default — controlled via `kcd pair` or IPC. // The controller is started in stopped state. diff --git a/internal/daemon/transport_dial.go b/internal/daemon/transport_dial.go index 595dad5..1170db6 100644 --- a/internal/daemon/transport_dial.go +++ b/internal/daemon/transport_dial.go @@ -53,7 +53,7 @@ func DialDevice(ctx context.Context, targetIP net.IP, targetPort int, targetID s logger.Debug("failed to dial peer", log.Error(err)) return } - transport.SetTCPKeepAlive(conn) + transport.SetTCPKeepAlive(conn, config.Duration(opts.Network.KeepAliveIdle)) var myID protocol.IdentityBody json.Unmarshal(identity.Body, &myID) diff --git a/internal/transport/keepalive.go b/internal/transport/keepalive.go index 4a4c737..7677185 100644 --- a/internal/transport/keepalive.go +++ b/internal/transport/keepalive.go @@ -5,31 +5,35 @@ import ( "time" ) -// TCP keepalive probe schedule, shared by the inbound listener and the -// outbound dial path. One definition keeps both ends symmetric — -// asymmetric timeouts are a classic source of one-sided zombie +// DefaultKeepAliveIdle is the first-probe delay when callers pass no +// explicit idle. Shared by the inbound listener and the outbound dial +// path — asymmetric timeouts are a classic source of one-sided zombie // connections where one end holds a dead socket the other has dropped. const ( - keepAliveIdle = 30 * time.Second - keepAliveInterval = 10 * time.Second - keepAliveCount = 3 + DefaultKeepAliveIdle = 30 * time.Second + keepAliveInterval = 10 * time.Second + keepAliveCount = 3 ) -// SetTCPKeepAlive enables keepalive probing on conn. Non-TCP connections -// are left untouched. Falls back to the legacy period-only API where the -// full config call is unavailable. -func SetTCPKeepAlive(conn net.Conn) { +// SetTCPKeepAlive enables keepalive probing on conn with the given idle +// delay. Non-positive idle falls back to DefaultKeepAliveIdle. Non-TCP +// connections are left untouched. Falls back to the legacy period-only +// API where the full config call is unavailable. +func SetTCPKeepAlive(conn net.Conn, idle time.Duration) { + if idle <= 0 { + idle = DefaultKeepAliveIdle + } tcpConn, ok := conn.(*net.TCPConn) if !ok { return } if err := tcpConn.SetKeepAliveConfig(net.KeepAliveConfig{ Enable: true, - Idle: keepAliveIdle, + Idle: idle, Interval: keepAliveInterval, Count: keepAliveCount, }); err != nil { _ = tcpConn.SetKeepAlive(true) - _ = tcpConn.SetKeepAlivePeriod(keepAliveIdle) + _ = tcpConn.SetKeepAlivePeriod(idle) } } diff --git a/internal/transport/listener.go b/internal/transport/listener.go index fb9fb88..3dd40a4 100644 --- a/internal/transport/listener.go +++ b/internal/transport/listener.go @@ -4,11 +4,15 @@ import ( "context" "fmt" "net" + "time" ) // Listener wraps a net.Listener. type Listener struct { l net.Listener + // keepAliveIdle overrides the first-probe delay for accepted + // connections; zero keeps DefaultKeepAliveIdle. + keepAliveIdle time.Duration } // Listen starts a TCP listener on the given TCP address. @@ -22,6 +26,12 @@ func Listen(ctx context.Context, addr string) (*Listener, error) { return &Listener{l: l}, nil } +// SetKeepAliveIdle overrides the keepalive first-probe delay for +// subsequently accepted connections. Must be called before serving. +func (l *Listener) SetKeepAliveIdle(d time.Duration) { + l.keepAliveIdle = d +} + // Accept waits for and returns the next connection. func (l *Listener) Accept() (net.Conn, error) { conn, err := l.l.Accept() @@ -29,7 +39,7 @@ func (l *Listener) Accept() (net.Conn, error) { return nil, err } - SetTCPKeepAlive(conn) + SetTCPKeepAlive(conn, l.keepAliveIdle) return conn, nil } diff --git a/internal/transport/sidechannel.go b/internal/transport/sidechannel.go index 5e45995..33383f0 100644 --- a/internal/transport/sidechannel.go +++ b/internal/transport/sidechannel.go @@ -20,6 +20,9 @@ import ( type SidechannelOptions struct { Timeout time.Duration IdleTimeout time.Duration + // KeepAliveIdle overrides the TCP keepalive first-probe delay for + // the dial; zero keeps DefaultKeepAliveIdle. + KeepAliveIdle time.Duration } // defaultSetupTimeout bounds side-channel establishment (TCP dial + TLS @@ -44,12 +47,15 @@ func DialSidechannel(ctx context.Context, ip net.IP, port int, tlsConfig *tls.Co if opts.Timeout <= 0 { opts.Timeout = defaultSetupTimeout } + if opts.KeepAliveIdle <= 0 { + opts.KeepAliveIdle = DefaultKeepAliveIdle + } addr := net.JoinHostPort(ip.String(), strconv.Itoa(port)) // One wall-clock budget covers everything before payload streaming: // the TCP connect and the TLS handshake together. setupCtx, cancel := context.WithTimeout(ctx, opts.Timeout) defer cancel() - dialer := net.Dialer{Timeout: opts.Timeout, KeepAlive: keepAliveIdle} + dialer := net.Dialer{Timeout: opts.Timeout, KeepAlive: opts.KeepAliveIdle} raw, err := dialer.DialContext(setupCtx, "tcp", addr) if err != nil { return nil, fmt.Errorf("side-channel: dial %s: %w", addr, err) diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index 278fc5b..e6b8af0 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -172,6 +172,8 @@ remotesystemvolume = true # handshake_timeout = "10s" # identity/TLS handshake # sidechannel_timeout = "15s" # side-channel connection setup # transfer_idle_timeout = "60s" # abort transfers silent longer than this +# keepalive_idle = "30s" # TCP keepalive first probe; >= 10s; + # larger = fewer probes, slower zombie detection [reconnect] # initial_backoff = "2s" From dd16d6a1cf1671f8ee5f8b6dd5fad049eb466d67 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 05:00:34 +0300 Subject: [PATCH 07/25] fix(mpris): reconcile heals state, expose posAnchorMs on mpris raw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconcile healed player names only: a missed or stale Play signal left the position poller disarmed with no further signal arriving to correct it. Reconcile now also refreshes tracked-player state (GetAll per player on rare, event-driven triggers — unknown senders, explicit queries, connects), sharing the extracted localStateChanged compare with the playing-poller tick. Read failures change nothing; nil bus is a safe no-op. Also exposes PosAnchorMs on ipc.MprisPlayerInfo: the daemon stamped every local broadcast, but the CLI decoded through a parallel struct that dropped the field, so kcd mpris raw never showed it. --- internal/ipc/proto.go | 1 + internal/plugins/mpris/playing_poller.go | 10 +--- internal/plugins/mpris/playing_poller_test.go | 17 +++++++ internal/plugins/mpris/reconcile.go | 51 +++++++++++++++++++ 4 files changed, 70 insertions(+), 9 deletions(-) diff --git a/internal/ipc/proto.go b/internal/ipc/proto.go index c5de5eb..86062d6 100644 --- a/internal/ipc/proto.go +++ b/internal/ipc/proto.go @@ -201,6 +201,7 @@ type MprisPlayerInfo struct { IsPlaying bool `json:"isPlaying"` Volume int `json:"volume"` Pos int64 `json:"pos"` + PosAnchorMs int64 `json:"posAnchorMs,omitempty"` Length int64 `json:"length"` AlbumArtUrl string `json:"albumArtUrl"` CanSeek bool `json:"canSeek"` diff --git a/internal/plugins/mpris/playing_poller.go b/internal/plugins/mpris/playing_poller.go index 4880992..1033ac8 100644 --- a/internal/plugins/mpris/playing_poller.go +++ b/internal/plugins/mpris/playing_poller.go @@ -89,15 +89,7 @@ func (p *MPRISPlugin) runPlayingPoller(ctx context.Context, interval time.Durati last := p.lastStates[pl.displayName] p.mu.RUnlock() - changed := last == nil || - state.PlaybackStatus != last.PlaybackStatus || - state.Title != last.Title || - state.Artist != last.Artist || - state.Album != last.Album || - state.AlbumArtUrl != last.AlbumArtUrl || - state.Volume != last.Volume || - state.IsPlaying != last.IsPlaying - if changed { + if localStateChanged(state, last) { p.storeLocalState(pl.displayName, state) p.broadcast(state) } diff --git a/internal/plugins/mpris/playing_poller_test.go b/internal/plugins/mpris/playing_poller_test.go index 3a83844..02488a4 100644 --- a/internal/plugins/mpris/playing_poller_test.go +++ b/internal/plugins/mpris/playing_poller_test.go @@ -90,6 +90,23 @@ func TestPollNeverArmsWhenDisabled(t *testing.T) { } } +// localStateChanged is the shared compare behind the poller tick and the +// reconcile state refresh: nil cache always counts, equal states don't. +func TestLocalStateChanged(t *testing.T) { + base := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T"} + if !localStateChanged(base, nil) { + t.Fatal("nil cache must count as changed") + } + same := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T"} + if localStateChanged(base, same) { + t.Fatal("equal states must not count as changed") + } + paused := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Paused", IsPlaying: false, Title: "T"} + if !localStateChanged(base, paused) { + t.Fatal("status flip must count as changed") + } +} + // Anchor is stamped on the broadcast state itself. func TestBroadcastAnchorFreshness(t *testing.T) { p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) diff --git a/internal/plugins/mpris/reconcile.go b/internal/plugins/mpris/reconcile.go index 5aa5471..24cc75c 100644 --- a/internal/plugins/mpris/reconcile.go +++ b/internal/plugins/mpris/reconcile.go @@ -52,6 +52,10 @@ func (p *MPRISPlugin) reconcilePlayers(conn *dbus.Conn, uniqueToDisplay map[stri add, drop := diffTracked(entries, tracked) if len(add) == 0 && len(drop) == 0 { + // Names agree, but state may still be stale (a missed Play + // signal disarms the position poller with nothing left to + // correct it). Heal that too. + p.refreshTrackedStates() return } p.logger.Debug("mpris: reconciling players") @@ -101,4 +105,51 @@ func (p *MPRISPlugin) reconcilePlayers(conn *dbus.Conn, uniqueToDisplay map[stri } } } + p.refreshTrackedStates() +} + +// refreshTrackedStates re-reads D-Bus state for every tracked player, +// storing and broadcasting changes. Reconcile heals names; this heals +// STATE: without it a missed or stale Play signal leaves the position +// poller disarmed (or a pause missed leaves it armed) with no further +// signal arriving to correct it. Triggers are rare (unknown senders, +// explicit queries, connects), so the per-player GetAll is event-driven +// cost — never a standing timer. Read failures change nothing. +func (p *MPRISPlugin) refreshTrackedStates() { + if p.dbus == nil { + return + } + p.mu.RLock() + names := make([]string, 0, len(p.players)) + for display := range p.players { + names = append(names, display) + } + p.mu.RUnlock() + + for _, display := range names { + state, err := p.playerState(display) + if err != nil { + continue + } + p.mu.RLock() + last := p.lastStates[display] + p.mu.RUnlock() + if localStateChanged(state, last) { + p.storeLocalState(display, state) + p.broadcast(state) + } + } +} + +// localStateChanged reports whether a fresh read differs from the cached +// state on any broadcasted field. A nil cache always counts as changed. +func localStateChanged(state, last *NowPlaying) bool { + return last == nil || + state.PlaybackStatus != last.PlaybackStatus || + state.Title != last.Title || + state.Artist != last.Artist || + state.Album != last.Album || + state.AlbumArtUrl != last.AlbumArtUrl || + state.Volume != last.Volume || + state.IsPlaying != last.IsPlaying } From 8b2a33f6ecc308e088b37344be36325a0d6b3df8 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 14:10:21 +0300 Subject: [PATCH 08/25] docs: post-release touch-ups (keepalive knob, MPRIS row, broadcast ownership) CLI.md claimed TCP keepalive was a fixed implementation setting; it is now tunable via network.keepalive_idle. AGENTS.md gains the missing MPRIS constructor row (including the new MPRISConfig param). ARCHITECTURE.md broadcast paragraph now describes the ownership model (pairing + reconnect owners, zero-owner steady state) and links the idle-behavior section. --- AGENTS.md | 1 + docs/ARCHITECTURE.md | 2 +- docs/CLI.md | 5 +++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5f2d890..9dfdf4e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -105,6 +105,7 @@ error message may be confusing. | SystemVolume | `systemvolume.NewSystemVolumePlugin(bus *events.Bus, logger log.Logger) *SystemVolumePlugin` | | SMS | `sms.NewSMSPlugin(cfg config.SMSConfig, bus *events.Bus, tlsConfig *tls.Config, logger log.Logger) *SMSPlugin` | | Contacts | `contacts.NewContactsPlugin(bus *events.Bus, logger log.Logger) *ContactsPlugin` | +| MPRIS | `mpris.NewMPRISPlugin(tlsConfig *tls.Config, bus *events.Bus, pauseMusic bool, mprisCfg config.MPRISConfig, logger log.Logger, cacheDirs ...string) *MPRISPlugin` | ### Interface diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 635b5fd..3203854 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -56,7 +56,7 @@ Devices are found via two parallel mechanisms that run concurrently: `Listener` binds to `0.0.0.0:1716` and parses every incoming UDP packet. Packets whose `type` is not `kdeconnect.identity` or whose `deviceId` matches the local device are silently dropped. -The broadcast interval is adaptive: when `shouldReduce()` returns true (all known devices already connected) the interval steps up to 60 seconds. Broadcast is controlled by a `BroadcasterController` which is off by default — it only starts during `kcd pair` (listen mode) and stops when pairing completes. The UDP **listener** is always active, so paired devices reconnect without any broadcast. +The broadcast interval is adaptive: when `shouldReduce()` returns true (all known devices already connected) the interval steps up to 60 seconds. Broadcast is controlled by a `BroadcasterController` which is off by default — it runs only while an owner holds it (`kcd pair` listen mode, or the reconnect watcher while any paired device is offline) and stops fully when the last owner withdraws, so connected steady state keeps zero timers. The UDP **listener** is always active. See [Idle behavior](#idle-behavior-zero-standing-timers) for the full zero-timer inventory. ### Ephemeral discovery dials diff --git a/docs/CLI.md b/docs/CLI.md index 3ddf867..f2c9f87 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -54,8 +54,9 @@ paths for overrides. Changing directories does not migrate existing files. `notifications.app_name` is reserved branding metadata, never a per-app filter. An explicit `ping.app_name = "KDE Connect"` in an older configuration remains an override even after changing the global name; remove it or set it to `""` to -inherit. Protocol version, payload limits, packet buffers, queues and TCP -keepalive remain fixed implementation settings, not configuration knobs. +inherit. Protocol version, payload limits, packet buffers and queues +remain fixed implementation settings, not configuration knobs. TCP +keepalive is tunable via `[network] keepalive_idle` (minimum `"10s"`). --- From 28f9c64da76ae26edb0d9d66ef2b43bd96a1da23 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 17:49:44 +0300 Subject: [PATCH 09/25] feat(aur): add kcd source package via aur_sources Second AUR target alongside kcd-bin: stable source builds with makedepends go, provides kcd, conflicts kcd-bin. Requires the aur_sources key (not aursources) and source.enabled for the release source tarball. aur-push.sh now loops both repos with the same options-patch and push flow. Verified with a snapshot build plus a full makepkg run in a disposable Arch container (namcap clean, package builds). --- .goreleaser.yaml | 55 +++++++++++++++++++++ scripts/aur-push.sh | 116 ++++++++++++++++++++++++-------------------- 2 files changed, 119 insertions(+), 52 deletions(-) diff --git a/.goreleaser.yaml b/.goreleaser.yaml index ec49bcc..d28e845 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -2,6 +2,10 @@ version: 2 project_name: kcd +# Source tarball for the AUR source package (aur_sources pipe). +source: + enabled: true + before: hooks: - go mod tidy @@ -110,6 +114,57 @@ aurs: install -Dm644 "./LICENSE" "${pkgdir}/usr/share/licenses/kcd-bin/LICENSE" +aur_sources: + - name: kcd + homepage: "https://github.com/bethropolis/kcd" + description: | + Lightweight, headless implementation of the KDE Connect protocol (v8) written in Go + maintainers: + - "bethropolis " + license: "MIT" + private_key: "{{ .Env.AUR_KEY }}" + skip_upload: true + git_url: "ssh://aur@aur.archlinux.org/kcd.git" + provides: + - kcd + conflicts: + - kcd-bin + makedepends: + - go + depends: + - glibc + optdepends: + - "libnotify: for desktop notifications" + - "wl-clipboard: for Wayland clipboard sync" + - "xclip: for X11 clipboard sync" + - "sshfs: for SFTP mounting support" + - "python-nautilus: for Nautilus file manager integration" + - "ydotool: for Wayland mousepad support" + - "xdotool: for X11 mousepad support" + - "wtype: for Wayland keyboard emulation" + build: |- + export CGO_ENABLED=0 + go build -trimpath -ldflags "-s -w -X main.version=${pkgver} -X main.commit={{.Commit}} -X main.date={{.Date}}" -o kcd ./cmd/kcd + package: |- + install -Dm755 "./kcd" "${pkgdir}/usr/bin/kcd" + + install -Dm644 "./packaging/kcd-pkg.service" "${pkgdir}/usr/lib/systemd/user/kcd.service" + install -Dm644 "./packaging/kcd-pkg.socket" "${pkgdir}/usr/lib/systemd/user/kcd.socket" + install -Dm644 "./packaging/kcd-system.service" "${pkgdir}/usr/lib/systemd/system/kcd@.service" + + install -Dm644 "./packaging/kcd.example.toml" "${pkgdir}/usr/share/doc/kcd/kcd.example.toml" + install -Dm644 "./README.md" "${pkgdir}/usr/share/doc/kcd/README.md" + install -Dm644 "./LICENSE" "${pkgdir}/usr/share/licenses/kcd/LICENSE" + + install -Dm644 "./packaging/kcd.bash-completion" "${pkgdir}/usr/share/bash-completion/completions/kcd" + install -Dm644 "./packaging/kcd.zsh-completion" "${pkgdir}/usr/share/zsh/site-functions/_kcd" + install -Dm644 "./packaging/kcd.fish-completion" "${pkgdir}/usr/share/fish/vendor_completions.d/kcd.fish" + + install -Dm644 "./packaging/nautilus-kcd.py" "${pkgdir}/usr/share/nautilus-python/extensions/nautilus-kcd.py" + + install -Dm644 "./packaging/firewalld-kcd.xml" "${pkgdir}/usr/lib/firewalld/services/kcd.xml" + install -Dm644 "./packaging/ufw-kcd" "${pkgdir}/etc/ufw/applications.d/kcd" + dockers_v2: - images: - "ghcr.io/bethropolis/kcd" diff --git a/scripts/aur-push.sh b/scripts/aur-push.sh index 3f85f77..87c2ad2 100755 --- a/scripts/aur-push.sh +++ b/scripts/aur-push.sh @@ -1,7 +1,7 @@ #!/bin/sh set -e -# Post-release hook: patch the GoReleaser-generated PKGBUILD with Arch +# Post-release hook: patch the GoReleaser-generated PKGBUILDs with Arch # options necessary to prevent debug split packages, then push to the # Arch User Repository (AUR) via the configured SSH key. # @@ -11,18 +11,18 @@ set -e # still generates the PKGBUILD/.SRCINFO with correct checksums and # sources, then this script patches them and handles the push. # +# Covers both the binary package (kcd-bin, from the `aurs` pipe) and the +# source package (kcd, from the `aur_sources` pipe). +# # Invoked by .github/workflows/release.yml after goreleaser finishes. AUR_DIR="${AUR_DIR:-dist/aur}" -AUR_PKGBUILD="${AUR_DIR}/kcd-bin.pkgbuild" -AUR_SRCINFO="${AUR_DIR}/kcd-bin.srcinfo" -AUR_SSH_URL="${AUR_SSH_URL:-ssh://aur@aur.archlinux.org/kcd-bin.git}" -AUR_CLONE="/tmp/kcd-aur-push" +AUR_CLONE_BASE="/tmp/kcd-aur-push" SSH_KEY_DIR="${SSH_KEY_DIR:-$(mktemp -d)}" SSH_KEY="${SSH_KEY_DIR}/aur_key" cleanup() { - rm -rf "${AUR_CLONE}" "${SSH_KEY_DIR}" + rm -rf "${AUR_CLONE_BASE}" "${SSH_KEY_DIR}" } trap cleanup EXIT @@ -32,57 +32,69 @@ if [ -z "${AUR_KEY}" ]; then exit 0 fi -# ── guard: PKGBUILD must exist (generated by goreleaser) ─────────────── -if [ ! -f "${AUR_PKGBUILD}" ]; then - echo "aur-push: ${AUR_PKGBUILD} not found — skipping (was goreleaser run?)" - exit 1 -fi - -# ── inject options into PKGBUILD (idempotent) ───────────────────────── -# The options= line prevents Arch's makepkg from creating a -debug split -# package. We add it right after the license= line so it sits in the -# metadata block where makepkg expects it. -if grep -q '^options=' "${AUR_PKGBUILD}"; then - echo "aur-push: options already present in PKGBUILD" -else - sed -i '/^license=/a options=('\''!strip'\'' '\''!debug'\'')' "${AUR_PKGBUILD}" - echo "aur-push: injected options into PKGBUILD" -fi - -# ── inject options into .SRCINFO (idempotent) ────────────────────────── -if grep -q '^ options = ' "${AUR_SRCINFO}"; then - echo "aur-push: options already present in .SRCINFO" -else - sed -i '/^ license = /a\\toptions = !strip !debug' "${AUR_SRCINFO}" - echo "aur-push: injected options into .SRCINFO" -fi - # ── set up SSH key for AUR push ──────────────────────────────────────── mkdir -p "${SSH_KEY_DIR}" echo "${AUR_KEY}" > "${SSH_KEY}" chmod 600 "${SSH_KEY}" SSH_CMD="ssh -i ${SSH_KEY} -o StrictHostKeyChecking=accept-new -o IdentitiesOnly=yes" -# ── clone AUR repo ───────────────────────────────────────────────────── -rm -rf "${AUR_CLONE}" -git clone -c core.sshCommand="${SSH_CMD}" "${AUR_SSH_URL}" "${AUR_CLONE}" +push_one() { + name="$1" # kcd-bin | kcd + ssh_url="$2" # AUR remote + pkgbuild="${AUR_DIR}/${name}.pkgbuild" + srcinfo="${AUR_DIR}/${name}.srcinfo" + clone_dir="${AUR_CLONE_BASE}/${name}" -# ── copy patched files ───────────────────────────────────────────────── -cp "${AUR_PKGBUILD}" "${AUR_CLONE}/PKGBUILD" -cp "${AUR_SRCINFO}" "${AUR_CLONE}/.SRCINFO" + # ── guard: PKGBUILD must exist (generated by goreleaser) ─────────── + if [ ! -f "${pkgbuild}" ] || [ ! -f "${srcinfo}" ]; then + echo "aur-push: ${pkgbuild} or ${srcinfo} not found — aborting ${name} (was goreleaser run?)" + exit 1 + fi -# ── commit and push ──────────────────────────────────────────────────── -cd "${AUR_CLONE}" -git add PKGBUILD .SRCINFO -if git diff --cached --quiet; then - echo "aur-push: no changes to AUR repo" -else - # Derive version from the PKGBUILD itself (avoid tag parsing fragility). - version=$(grep -oP "^pkgver=\K.+" PKGBUILD | head -1 | sed 's/_/-/g') - git \ - -c user.name="kcd bot" \ - -c user.email="bot@kcd.invalid" \ - commit -m "Update to v${version}" - git push - echo "aur-push: pushed v${version} to AUR" -fi + # ── inject options into PKGBUILD (idempotent) ───────────────────── + # The options= line prevents Arch's makepkg from creating a -debug split + # package. We add it right after the license= line so it sits in the + # metadata block where makepkg expects it. + if grep -q '^options=' "${pkgbuild}"; then + echo "aur-push: options already present in ${name} PKGBUILD" + else + sed -i '/^license=/a options=('\''!strip'\'' '\''!debug'\'')' "${pkgbuild}" + echo "aur-push: injected options into ${name} PKGBUILD" + fi + + # ── inject options into .SRCINFO (idempotent) ────────────────────── + if grep -q '^ options = ' "${srcinfo}"; then + echo "aur-push: options already present in ${name} .SRCINFO" + else + sed -i '/^ license = /a\\toptions = !strip !debug' "${srcinfo}" + echo "aur-push: injected options into ${name} .SRCINFO" + fi + + # ── clone AUR repo ───────────────────────────────────────────────── + rm -rf "${clone_dir}" + git clone -c core.sshCommand="${SSH_CMD}" "${ssh_url}" "${clone_dir}" + + # ── copy patched files ───────────────────────────────────────────── + cp "${pkgbuild}" "${clone_dir}/PKGBUILD" + cp "${srcinfo}" "${clone_dir}/.SRCINFO" + + # ── commit and push ──────────────────────────────────────────────── + cd "${clone_dir}" + git add PKGBUILD .SRCINFO + if git diff --cached --quiet; then + echo "aur-push: no changes to ${name} AUR repo" + else + # Derive version from the PKGBUILD itself (avoid tag parsing fragility). + version=$(grep -oP "^pkgver=\K.+" PKGBUILD | head -1 | sed 's/_/-/g') + git \ + -c user.name="kcd bot" \ + -c user.email="bot@kcd.invalid" \ + commit -m "Update to v${version}" + git push + echo "aur-push: pushed v${version} to ${name} AUR repo" + fi + cd - > /dev/null +} + +push_one "kcd-bin" "${AUR_SSH_URL:-ssh://aur@aur.archlinux.org/kcd-bin.git}" +push_one "kcd" "${AUR_SSH_URL_SRC:-ssh://aur@aur.archlinux.org/kcd.git}" From e74584285c1dbe5a40b4291c53082898cc72fc01 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Tue, 22 Sep 2026 18:04:33 +0300 Subject: [PATCH 10/25] docs(release): mention Arch source package in install notes --- .goreleaser.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.goreleaser.yaml b/.goreleaser.yaml index d28e845..31803d7 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -317,6 +317,12 @@ release: systemctl --user enable --now kcd.socket ``` + **Arch Linux source:** + ```bash + yay -S kcd + systemctl --user enable --now kcd.socket + ``` + **Debian / Ubuntu:** ```bash curl -LO https://github.com/bethropolis/kcd/releases/download/v{{ .Version }}/kcd_{{ .Version }}_x86_64.deb From e64218ba77f60062d6a95be9202e1d2d926bc874 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:17:18 +0300 Subject: [PATCH 11/25] fix: address Copilot review findings on idle-timer release Six review findings plus a crash found by the integration gate: - mpris: stamp PosAnchorMs and snapshot the broadcast state under p.mu; the stamp aliased the lastStates pointer that DebugStatus reads from the IPC path, and the new race test reproduces both the stamp/read race and concurrent-broadcast marshal race under -race. - config: enforce reconnect.fallback_max >= max_backoff only when sighting_driven is true. Legacy configs that raised max_backoff above the new 1h default previously failed to load; fallback_max is unused in legacy mode. - reconnect: reload the dial target from LastSightedIP on every parked loop lap, so a roam survives a failed one-shot dial instead of falling back to the stale spawn-time address. - discovery: SetBrowseStarter now attaches to an already-running owned loop, closing the startup window where reconnect ownership began before the starter was registered (mDNS-only networks stayed silent). - discovery: zeroconf closes the entries channel itself on ctx cancel; our defer close(entries) double-closed it and panicked whenever an owned browse was cancelled (caught by integration -race). - mpris: include position drift past 3s in the local change comparison. Steady playback stays silent because the phone extrapolates from the anchor; seeks, stalls, and missed signals now re-broadcast. - events: add OnSubscriberChange hooks; the remote MPRIS poller ticker now exists only while a subscriber listens for mpris.update instead of waking every 5s with the request suppressed. Gate: gofmt, vet, unit, race on touched packages, integration -race, static build all green. golangci-lint cannot run on this machine (its embedded go/types is built with go1.26 while the toolchain is go1.27; pre-existing, fails identically on the untouched tree). --- docs/ARCHITECTURE.md | 4 +- docs/CLI.md | 2 +- docs/CLIENT_GUIDE.md | 18 +++--- docs/IPC_PROTOCOL.md | 9 +-- internal/config/config_test.go | 10 ++++ internal/config/runtime.go | 2 +- internal/daemon/transport.go | 8 ++- internal/daemon/transport_reconnect.go | 13 ++++- internal/daemon/transport_reconnect_test.go | 50 ++++++++++++++++ internal/device/device_discovery.go | 12 ++++ internal/discovery/broadcaster_controller.go | 16 ++++- internal/discovery/discovery_test.go | 31 ++++++++++ internal/discovery/listener_mdns.go | 3 +- internal/events/bus.go | 34 ++++++++++- internal/events/bus_test.go | 24 ++++++++ internal/plugins/mpris/local.go | 11 +++- internal/plugins/mpris/mpris.go | 11 +++- internal/plugins/mpris/mpris_test.go | 44 ++++++++++++++ internal/plugins/mpris/playing_poller.go | 6 +- internal/plugins/mpris/playing_poller_test.go | 58 +++++++++++++++++-- internal/plugins/mpris/reconcile.go | 40 +++++++++++-- internal/plugins/mpris/remote.go | 51 +++++++++++----- internal/plugins/mpris/remote_gate_test.go | 30 ++++++++++ 23 files changed, 435 insertions(+), 52 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 3203854..7c595cb 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -91,8 +91,8 @@ Connected steady state (all pairs connected, nothing playing, no transfers, no p | mDNS browse | Same owned lifetime as broadcast (probes are periodic by library design) | | mDNS advertise | Lifetime-on, responder-only (no timers) | | UDP/TCP/IPC listeners, D-Bus signals, bus subscriptions | Blocking waits, zero CPU until an event arrives | -| Local position poller | Exists only while ≥1 local player `IsPlaying` (`[mpris] poll_while_playing`, `position_interval`) | -| Remote state poller | Fires only while a client subscribes to `mpris.update` | +| Local position poller | Exists only while ≥1 local player `IsPlaying` (`[mpris] poll_while_playing`, `position_interval`); ticks re-broadcast only on metadata change or position drift >3s off the anchor extrapolation | +| Remote state poller | Ticker itself exists only while a client subscribes to `mpris.update` (bus subscriber-change hook starts/stops it) | | Reconnect redial | Parked on discovery sightings; fallback escalates to `fallback_max`, then gives up past `stale_after` until the next sighting | | TCP keepalive | Kernel probes, first delay `[network] keepalive_idle` (default 30s, minimum 10s) | diff --git a/docs/CLI.md b/docs/CLI.md index f2c9f87..fcc8680 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -534,7 +534,7 @@ kcd watch --events=mpris.update # bindsym XF86AudioNext exec kcd mpris next ``` -> **Note:** The `mpris.update` event fires whenever the phone sends a now-playing state change (track change, play/pause toggle). Subscribe with `kcd watch --events=mpris.update`. The daemon also re-requests now-playing every 5 seconds from devices with an **actively-playing** player, so state stays fresh for pure-push clients without polling — but events are deduplicated, so `mpris.update` only fires on real changes. Stopped/paused players are not polled (a stopped-but-alive track stays listed so it can be resumed), and when the phone removes a player from its `playerList` (session destroyed) the cached state is dropped and an empty `mpris.update` is emitted so the widget falls back to "no media playing". +> **Note:** The `mpris.update` event fires whenever the phone sends a now-playing state change (track change, play/pause toggle). Subscribe with `kcd watch --events=mpris.update`. While subscribed, the daemon runs a 5-second ticker that re-requests now-playing from devices with an **actively-playing** player, so state stays fresh for pure-push clients without polling — but events are deduplicated, so `mpris.update` only fires on real changes. The ticker stops with the last unsubscriber, so nothing ticks unobserved. Stopped/paused players are not polled (a stopped-but-alive track stays listed so it can be resumed), and when the phone removes a player from its `playerList` (session destroyed) the cached state is dropped and an empty `mpris.update` is emitted so the widget falls back to "no media playing". > > **Note:** Phone album art URIs (`kdeconnect:/artUri?...`) are resolved by the > daemon: it fetches the art bytes from the phone, caches them to diff --git a/docs/CLIENT_GUIDE.md b/docs/CLIENT_GUIDE.md index 6b4c014..7850803 100644 --- a/docs/CLIENT_GUIDE.md +++ b/docs/CLIENT_GUIDE.md @@ -295,11 +295,12 @@ except KeyboardInterrupt: | `ping.received` | Ping from device | > **Freshness:** while at least one client watches `mpris.update`, the -> daemon re-requests now-playing from devices with an actively-playing -> player every 5 seconds, so a pure-push client (a widget watching the -> event stream, with no polling) receives the current track within one poll -> interval of subscribing — including mid-track mount, thanks to the initial -> event dump. With nobody watching, no refresh requests go out at all. +> daemon runs a 5-second ticker that re-requests now-playing from devices +> with an actively-playing player, so a pure-push client (a widget watching +> the event stream, with no polling) receives the current track within one +> poll interval of subscribing — including mid-track mount, thanks to the +> initial event dump. The ticker itself only runs while watched: with +> nobody watching, there is no timer and no refresh requests go out at all. > Events are deduplicated: `mpris.update` only fires when the > state actually changed, so the stream stays quiet between track changes. > Stopped/paused players are not polled, and when the phone removes a player @@ -319,8 +320,11 @@ except KeyboardInterrupt: > > **Local idle behavior:** the D-Bus watcher is event-driven, but while a > local player is playing the daemon re-reads its state every -> `position_interval` (default `"2s"`, `[mpris]` section) so the phone's -> now-playing display stays exact. Nothing is polled while paused or with +> `position_interval` (default `"2s"`, `[mpris]` section). Steady playback +> stays silent — the phone extrapolates from `posAnchorMs` — and a tick +> re-broadcasts only on a metadata change or when the true position drifts +> more than 3s off the extrapolation (seek, missed signal, clock drift). +> Nothing is polled while paused or with > no players — a silent desktop costs zero wakeups. Set > `poll_while_playing = false` for pure event-driven mode (position then > extrapolates from `posAnchorMs` between D-Bus signals). diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index ac0ca32..f16d1db 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -1242,10 +1242,11 @@ Now-playing state from a device's media player. > clears on the next state change. > **Freshness:** while at least one client subscribes to `mpris.update`, -> the daemon re-requests now-playing from every connected -> device with an **actively-playing** player every 5 seconds -> (`kdeconnect.mpris.request` with `requestNowPlaying: true`). With nobody -> subscribed, no refresh requests go out. Responses are +> the daemon runs a 5-second ticker that re-requests now-playing from +> every connected device with an **actively-playing** player +> (`kdeconnect.mpris.request` with `requestNowPlaying: true`). The ticker +> itself only exists while subscribed — with nobody listening there is no +> timer and no refresh requests go out. Responses are > deduplicated — an event is only emitted when the state actually changes. > This keeps `pos`/state current for pure-push clients (widgets, Waybar) > that never poll the CLI. Devices that haven't reported a player yet, or diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 0405ed6..87172b9 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -182,6 +182,16 @@ func TestFallbackMaxRelationship(t *testing.T) { if _, err := loadTOML(t, "[reconnect]\nmax_backoff = '5m'\nfallback_max = '1h'\n"); err != nil { t.Fatalf("wider fallback_max should be valid: %v", err) } + // Pre-1.20 configs that raised max_backoff above the new 1h fallback + // default must keep loading in legacy mode: fallback_max is unused + // with sighting_driven=false, so the relationship is not enforced. + if _, err := loadTOML(t, "[reconnect]\nsighting_driven = false\nmax_backoff = '2h'\n"); err != nil { + t.Fatalf("legacy max_backoff above default fallback must stay valid: %v", err) + } + if _, err := loadTOML(t, "[reconnect]\nsighting_driven = true\nmax_backoff = '2h'\n"); err == nil || + !strings.Contains(err.Error(), "reconnect.fallback_max") { + t.Fatalf("expected fallback_max relationship error when sighting-driven, got %v", err) + } } func TestKeepAliveIdleFloor(t *testing.T) { diff --git a/internal/config/runtime.go b/internal/config/runtime.go index 9eccb91..cf28cda 100644 --- a/internal/config/runtime.go +++ b/internal/config/runtime.go @@ -94,7 +94,7 @@ func (c *Config) validateDurations() error { if Duration(c.Reconnect.MaxBackoff) < Duration(c.Reconnect.InitialBackoff) { return fmt.Errorf("config: reconnect.max_backoff must be >= reconnect.initial_backoff") } - if Duration(c.Reconnect.FallbackMax) < Duration(c.Reconnect.MaxBackoff) { + if c.Reconnect.SightingDriven && Duration(c.Reconnect.FallbackMax) < Duration(c.Reconnect.MaxBackoff) { return fmt.Errorf("config: reconnect.fallback_max must be >= reconnect.max_backoff") } if Duration(c.Network.KeepAliveIdle) < 10*time.Second { diff --git a/internal/daemon/transport.go b/internal/daemon/transport.go index 95e2b29..b15e4d9 100644 --- a/internal/daemon/transport.go +++ b/internal/daemon/transport.go @@ -185,9 +185,11 @@ func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.Broadcaste // Wake a parked reconnect loop (peer provably alive at // this address), or respawn one that gave up past the // stale horizon. TryReconnect single-flights: exactly one - // loop per device. The sighted address replaces a stale - // LastIP, and the attempt counter restarts — the peer is - // provably back, so escalated backoff no longer applies. + // loop per device. The sighting is recorded as the freshest + // known address (the parked loop reloads it every lap, so a + // roam survives a failed one-shot dial), and the attempt + // counter restarts — the peer is provably back, so escalated + // backoff no longer applies. dev.PokeReconnect() if !dev.IsConnected() && dev.TryReconnect() { dev.ResetReconnectAttempt() diff --git a/internal/daemon/transport_reconnect.go b/internal/daemon/transport_reconnect.go index 5aaf8ee..05cd2e2 100644 --- a/internal/daemon/transport_reconnect.go +++ b/internal/daemon/transport_reconnect.go @@ -131,9 +131,18 @@ func reconnectWithBackoff( continue } + // Reload the dial target every lap: a sighting while parked + // records a fresher address (roam) than this loop's spawn-time + // target, and the one-shot discovery dial may have failed. The + // spawn address stays the fallback when nothing was ever sighted. + dialIP := ip + if sighted := dev.LastSightedIP(); sighted != nil { + dialIP = sighted + } + logger.Info("auto-reconnect: dialling", log.String("device_id", dev.ID()), - log.String("ip", ip.String()), + log.String("ip", dialIP.String()), log.Int("attempt", attempt+1), log.Bool("sighting_triggered", triggered), ) @@ -142,7 +151,7 @@ func reconnectWithBackoff( // default: the identity may carry a non-standard port (or none // at all, in which case LastPort is 0 and we fall back). port := reconnectPort(dev, opts.TCPPort) - DialDevice(ctx, ip, port, dev.ID(), protocol.ProtocolVersion, identity, cfg, devices, plugins, localDeviceID, logger, false, opts) + DialDevice(ctx, dialIP, port, dev.ID(), protocol.ProtocolVersion, identity, cfg, devices, plugins, localDeviceID, logger, false, opts) lastDial = time.Now() if dev.IsConnected() { diff --git a/internal/daemon/transport_reconnect_test.go b/internal/daemon/transport_reconnect_test.go index e39cdda..9b20060 100644 --- a/internal/daemon/transport_reconnect_test.go +++ b/internal/daemon/transport_reconnect_test.go @@ -160,6 +160,56 @@ func TestReconnectUnpairExitsParkedLoop(t *testing.T) { waitForReconnectExit(t, dev, 5*time.Second) } +// A roam sighting while parked must redirect fallback dials: the loop is +// spawned with a dead spawn-time target, a sighting is recorded at the +// live listener address, and the poked loop must dial the sighted +// address — not keep redialling the stale spawn target. +func TestReconnectFollowsSightedAddress(t *testing.T) { + ln, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.2:0") + if err != nil { + t.Fatal(err) + } + var count atomic.Int32 + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + count.Add(1) + c.Close() + } + }() + t.Cleanup(func() { ln.Close() }) + + dev := parkedDevice(t, "roam-2") + dev.SetLastPort(ln.Addr().(*net.TCPAddr).Port) + + opts := parkedOpts() + identity, err := newTestIdentity() + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + // Spawn target is dead (refused): only the sighted address can answer. + go reconnectWithBackoff(ctx, dev, net.ParseIP("127.0.0.1"), identity, + &tls.Config{InsecureSkipVerify: true}, device.NewRegistry(nil), + plugin.NewRegistry(log.Nop()), "local", log.Nop(), opts) + + time.Sleep(100 * time.Millisecond) // let the loop park on the timer + dev.NoteSighting(net.ParseIP("127.0.0.2")) + dev.PokeReconnect() + + deadline := time.Now().Add(5 * time.Second) + for count.Load() == 0 && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if count.Load() == 0 { + t.Fatal("parked loop did not dial the sighted address within 5s") + } +} + // Legacy mode (sighting_driven=false) keeps pure-timer dials capped at // max_backoff: the timer path must still fire without any sighting. func TestReconnectLegacyTimerStillDials(t *testing.T) { diff --git a/internal/device/device_discovery.go b/internal/device/device_discovery.go index fdb7315..62e0728 100644 --- a/internal/device/device_discovery.go +++ b/internal/device/device_discovery.go @@ -144,6 +144,18 @@ func (d *Device) NoteSighting(sighted net.IP) (roamed bool) { return false } +// LastSightedIP returns the most recent discovery sighting address, or +// nil if the peer has never announced itself in this process. Unlike +// LastIP (the last *authenticated* address), this tracks where the peer +// provably is right now — the parked reconnect loop prefers it over its +// spawn-time target so fallback attempts follow roams instead of +// redialling a stale address after a failed one-shot dial. +func (d *Device) LastSightedIP() net.IP { + d.mu.RLock() + defer d.mu.RUnlock() + return d.lastSightedIP +} + // TryReconnect attempts to mark the device as reconnecting. // Returns true if this goroutine should proceed; false if another // reconnect goroutine is already running. diff --git a/internal/discovery/broadcaster_controller.go b/internal/discovery/broadcaster_controller.go index 8c8d68a..15a2b78 100644 --- a/internal/discovery/broadcaster_controller.go +++ b/internal/discovery/broadcaster_controller.go @@ -30,7 +30,12 @@ type BroadcasterController struct { mu sync.Mutex running bool cancel context.CancelFunc - owners map[string]struct{} + // ownedCtx is the context the owned loop (and its browse companion) + // runs under. Retained so a late-registered browse starter can + // attach to an already-running owner instead of waiting for the + // next ownership cycle. + ownedCtx context.Context + owners map[string]struct{} // browseStarter, when set, launches mDNS browsing on the same owned // context as the broadcast loop: active discovery (both directions) // shares one lifetime — pairing/reconnect only, never steady state. @@ -73,11 +78,17 @@ func (bc *BroadcasterController) Stop() { } // SetBrowseStarter registers the mDNS browse function to run alongside -// the owned broadcast loop. Called once at startup before any StartOwned. +// the owned broadcast loop. Called once at startup; if an owner already +// holds the loop (startup reconnect ownership racing transport setup), +// the starter launches immediately on the owned context instead of +// waiting for the next ownership cycle. func (bc *BroadcasterController) SetBrowseStarter(starter func(ctx context.Context)) { bc.mu.Lock() defer bc.mu.Unlock() bc.browseStarter = starter + if bc.running && bc.ownedCtx != nil && starter != nil { + go starter(bc.ownedCtx) + } } // StartOwned launches the loop (if needed) and records owner as needing it. @@ -93,6 +104,7 @@ func (bc *BroadcasterController) StartOwned(parentCtx context.Context, owner str ctx, cancel := context.WithCancel(parentCtx) bc.cancel = cancel bc.running = true + bc.ownedCtx = ctx b := &Broadcaster{ identityPacket: bc.identityPacket, diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go index 6f9787d..0957865 100644 --- a/internal/discovery/discovery_test.go +++ b/internal/discovery/discovery_test.go @@ -91,6 +91,37 @@ func TestBrowseStarterSharesOwnership(t *testing.T) { } } +// Startup race: reconnect ownership can begin before transport setup +// registers the browse starter. A late setter must attach browsing to +// the already-running owner instead of waiting for a cycle that may +// never come (the pair is already offline and silent). +func TestLateBrowseStarterAttachesToRunningOwner(t *testing.T) { + bc := NewBroadcasterController(testIdentity(t), protocol.DefaultTCPPort, time.Hour, log.Nop(), nil) + ctx := context.Background() + + bc.StartOwned(ctx, OwnerReconnect) + if !bc.IsRunning() { + t.Fatal("loop must run before the starter is registered") + } + + started := make(chan context.Context, 1) + bc.SetBrowseStarter(func(browseCtx context.Context) { + started <- browseCtx + }) + + select { + case browseCtx := <-started: + select { + case <-browseCtx.Done(): + t.Fatal("attached browse context must live while the owner holds the loop") + default: + } + case <-time.After(2 * time.Second): + t.Fatal("late browse starter did not attach to the running owner") + } + bc.StopOwned(OwnerReconnect) +} + // Without a registered starter the controller behaves exactly as before. func TestNoBrowseStarterNoBrowse(t *testing.T) { bc := NewBroadcasterController(testIdentity(t), protocol.DefaultTCPPort, time.Hour, log.Nop(), nil) diff --git a/internal/discovery/listener_mdns.go b/internal/discovery/listener_mdns.go index 96ee105..fb7bce9 100644 --- a/internal/discovery/listener_mdns.go +++ b/internal/discovery/listener_mdns.go @@ -56,10 +56,9 @@ func AdvertiseMDNS(ctx context.Context, identityPacket *protocol.Packet, logger // plus a 10s cache-cleanup ticker, which is incompatible with zero-idle // steady state. Connected steady state relies on the lifetime UDP listener // and mDNS advertisement for inbound discovery instead. Returns when ctx -// ends; the entries channel is closed so the results loop exits too. +// ends; zeroconf closes the entries channel, which ends the results loop. func (l *Listener) RunMdnsDiscovery(ctx context.Context) { entries := make(chan *zeroconf.ServiceEntry) - defer close(entries) go func(results <-chan *zeroconf.ServiceEntry) { for entry := range results { var deviceId, deviceName, deviceType string diff --git a/internal/events/bus.go b/internal/events/bus.go index 1bc6621..6bd0de1 100644 --- a/internal/events/bus.go +++ b/internal/events/bus.go @@ -93,6 +93,10 @@ type Bus struct { subscribers map[uint64]*Subscriber nextID uint64 logger log.Logger + // changeHooks run (without the bus lock held) after every subscribe + // and unsubscribe, so demand-driven producers can start/stop with + // the audience instead of polling HasSubscribers on a timer. + changeHooks []func() } // NewBus creates a new event bus. @@ -108,7 +112,6 @@ func NewBus(logger log.Logger) *Bus { // If filters is empty, it receives all events. func (b *Bus) Subscribe(capacity int, filters ...EventType) *Subscriber { b.mu.Lock() - defer b.mu.Unlock() b.nextID++ id := b.nextID @@ -127,18 +130,45 @@ func (b *Bus) Subscribe(capacity int, filters ...EventType) *Subscriber { b.subscribers[id] = sub b.logger.Debug("new subscriber", log.Uint64("id", id), log.Int("filters", len(filters))) + b.mu.Unlock() + b.notifyChange() return sub } // unsubscribe removes a subscriber. func (b *Bus) unsubscribe(id uint64) { b.mu.Lock() - defer b.mu.Unlock() + removed := false if sub, ok := b.subscribers[id]; ok { close(sub.ch) delete(b.subscribers, id) b.logger.Debug("subscriber removed", log.Uint64("id", id)) + removed = true + } + b.mu.Unlock() + if removed { + b.notifyChange() + } +} + +// OnSubscriberChange registers a hook invoked after every subscribe and +// unsubscribe. Hooks run without the bus lock held and must return +// quickly; they typically re-check HasSubscribers and start/stop a +// producer. Register before subscribers arrive — hooks do not replay. +func (b *Bus) OnSubscriberChange(fn func()) { + b.mu.Lock() + b.changeHooks = append(b.changeHooks, fn) + b.mu.Unlock() +} + +func (b *Bus) notifyChange() { + b.mu.RLock() + hooks := make([]func(), len(b.changeHooks)) + copy(hooks, b.changeHooks) + b.mu.RUnlock() + for _, fn := range hooks { + fn() } } diff --git a/internal/events/bus_test.go b/internal/events/bus_test.go index eba22ab..6526a9c 100644 --- a/internal/events/bus_test.go +++ b/internal/events/bus_test.go @@ -1,6 +1,7 @@ package events import ( + "sync/atomic" "testing" "github.com/bethropolis/kcd/internal/log" @@ -34,3 +35,26 @@ func TestHasSubscribers(t *testing.T) { t.Fatal("unfiltered subscriber must match every type") } } + +func TestOnSubscriberChange(t *testing.T) { + b := NewBus(log.Nop()) + + var calls atomic.Int32 + b.OnSubscriberChange(func() { calls.Add(1) }) + + sub := b.Subscribe(4, TypeMprisUpdate) + if n := calls.Load(); n != 1 { + t.Fatalf("subscribe fired hook %d times, want 1", n) + } + sub.Close() + if n := calls.Load(); n != 2 { + t.Fatalf("unsubscribe fired hook %d times total, want 2", n) + } + + // Double close is a no-op for the subscriber map and must not + // re-fire the hook. + sub.Close() + if n := calls.Load(); n != 2 { + t.Fatalf("duplicate close fired hook, total %d want 2", n) + } +} diff --git a/internal/plugins/mpris/local.go b/internal/plugins/mpris/local.go index 971c0bd..90727c6 100644 --- a/internal/plugins/mpris/local.go +++ b/internal/plugins/mpris/local.go @@ -73,9 +73,18 @@ func (p *MPRISPlugin) broadcast(state *NowPlaying) { // caller (signal-time query or poller GetAll) immediately before // this broadcast, so receivers can extrapolate the live position as // Pos + (nowMs - PosAnchorMs) while IsPlaying. + // + // The stamp takes p.mu, and the packet is marshalled from a snapshot + // taken under the same hold: state aliases the pointer cached in + // lastStates, and DebugStatus reads its anchor under RLock from the + // IPC path. Stamping without the mutex races those reads, and + // marshalling the live pointer races a concurrent broadcast's stamp. + p.mu.Lock() state.PosAnchorMs = time.Now().UnixMilli() + snapshot := *state + p.mu.Unlock() - pkt, err := protocol.NewPacket("kdeconnect.mpris", state) + pkt, err := protocol.NewPacket("kdeconnect.mpris", &snapshot) if err != nil { return } diff --git a/internal/plugins/mpris/mpris.go b/internal/plugins/mpris/mpris.go index f152661..bb4ad09 100644 --- a/internal/plugins/mpris/mpris.go +++ b/internal/plugins/mpris/mpris.go @@ -39,6 +39,11 @@ type MPRISPlugin struct { mprisCfg config.MPRISConfig pollCancel context.CancelFunc + // remotePollCancel stops the remote-state poller; nil means it is + // not running. The poller is demand-driven (see syncRemotePoller): + // no mpris.update subscribers, no ticker — zero idle timers. + remotePollCancel context.CancelFunc + // reconcileCh nudges the D-Bus watcher loop to re-list player names // and heal drift. Buffered size 1 so bursts of triggers coalesce; // sends are non-blocking. Event-driven only — no timers. @@ -103,7 +108,11 @@ func NewMPRISPlugin(tlsConfig *tls.Config, bus *events.Bus, pauseMusic bool, mpr p.watchCancel = cancel p.watching = true p.startWatcher(watchCtx) - p.startRemoteStatePoller(watchCtx) + // The remote-state poller follows the audience: the bus hook starts + // it on the first mpris.update subscriber and stops it on the last + // unsubscribe, so the 5s ticker never runs unobserved. + p.bus.OnSubscriberChange(p.syncRemotePoller) + p.syncRemotePoller() // Subscribe to telephony events for pause-music-on-call. if p.pauseMusic && p.dbus != nil { diff --git a/internal/plugins/mpris/mpris_test.go b/internal/plugins/mpris/mpris_test.go index ca661e2..2c5a5b3 100644 --- a/internal/plugins/mpris/mpris_test.go +++ b/internal/plugins/mpris/mpris_test.go @@ -509,3 +509,47 @@ func TestPublishedEventHasAnchorAndPendingArt(t *testing.T) { t.Errorf("anchor mismatch: published %d, served %d", pub.PosAnchorMs, rs.PosAnchorMs) } } + +// Broadcast stamps the anchor on the same pointer cached in lastStates +// while DebugStatus reads that cached anchor under RLock from the IPC +// path. Hammer both concurrently: under -race any unsynchronized stamp +// is reported. The locked RLock read mirrors DebugStatus's access. +func TestBroadcastAnchorStampRace(t *testing.T) { + bus := events.NewBus(log.Nop()) + plugin := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) + if plugin.watchCancel != nil { + defer plugin.watchCancel() + } + + state := &NowPlaying{Player: "Racer", Title: "T", Pos: 1000, IsPlaying: true} + plugin.mu.Lock() + plugin.lastStates["Racer"] = state + plugin.mu.Unlock() + + var wg sync.WaitGroup + for i := 0; i < 4; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 50; j++ { + plugin.broadcast(state) + } + }() + } + for i := 0; i < 4; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 50; j++ { + plugin.mu.RLock() + _ = plugin.lastStates["Racer"].PosAnchorMs + plugin.mu.RUnlock() + } + }() + } + wg.Wait() + + if state.PosAnchorMs == 0 { + t.Error("expected broadcast to stamp PosAnchorMs") + } +} diff --git a/internal/plugins/mpris/playing_poller.go b/internal/plugins/mpris/playing_poller.go index 1033ac8..82dc4c7 100644 --- a/internal/plugins/mpris/playing_poller.go +++ b/internal/plugins/mpris/playing_poller.go @@ -85,11 +85,13 @@ func (p *MPRISPlugin) runPlayingPoller(ctx context.Context, interval time.Durati if err != nil { continue } + // Hold RLock through the compare: it reads the cached + // anchor, which broadcast stamps under p.mu. p.mu.RLock() - last := p.lastStates[pl.displayName] + changed := localStateChanged(time.Now().UnixMilli(), state, p.lastStates[pl.displayName]) p.mu.RUnlock() - if localStateChanged(state, last) { + if changed { p.storeLocalState(pl.displayName, state) p.broadcast(state) } diff --git a/internal/plugins/mpris/playing_poller_test.go b/internal/plugins/mpris/playing_poller_test.go index 02488a4..c512383 100644 --- a/internal/plugins/mpris/playing_poller_test.go +++ b/internal/plugins/mpris/playing_poller_test.go @@ -91,22 +91,72 @@ func TestPollNeverArmsWhenDisabled(t *testing.T) { } // localStateChanged is the shared compare behind the poller tick and the -// reconcile state refresh: nil cache always counts, equal states don't. +// reconcile state refresh: nil cache always counts, equal states don't, +// and position counts only on drift past the tolerance. func TestLocalStateChanged(t *testing.T) { + now := time.Now().UnixMilli() base := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T"} - if !localStateChanged(base, nil) { + if !localStateChanged(now, base, nil) { t.Fatal("nil cache must count as changed") } same := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T"} - if localStateChanged(base, same) { + if localStateChanged(now, base, same) { t.Fatal("equal states must not count as changed") } paused := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Paused", IsPlaying: false, Title: "T"} - if !localStateChanged(base, paused) { + if !localStateChanged(now, base, paused) { t.Fatal("status flip must count as changed") } } +// Steady playback inside the tolerance must stay silent: the phone +// extrapolates from the anchor, so an on-schedule tick is not a change. +func TestLocalStateChangedSteadyPlaybackSilent(t *testing.T) { + now := time.Now().UnixMilli() + last := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T", Pos: 40000, PosAnchorMs: now - 2000} + onSchedule := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T", Pos: 42000} + if localStateChanged(now, onSchedule, last) { + t.Fatal("on-schedule position must not count as changed") + } +} + +// A seek jumps off the extrapolation: the tick must re-broadcast and +// refresh the phone's anchor. +func TestLocalStateChangedSeekCounts(t *testing.T) { + now := time.Now().UnixMilli() + last := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T", Pos: 40000, PosAnchorMs: now - 2000} + seeked := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T", Pos: 90000} + if !localStateChanged(now, seeked, last) { + t.Fatal("seek off the extrapolation must count as changed") + } +} + +// A stall (position not advancing though playing) drifts off the +// extrapolation the other way and must also refresh. +func TestLocalStateChangedStallCounts(t *testing.T) { + now := time.Now().UnixMilli() + last := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T", Pos: 40000, PosAnchorMs: now - 10000} + stalled := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T", Pos: 42000} + if !localStateChanged(now, stalled, last) { + t.Fatal("stalled position must count as changed") + } +} + +// Paused players don't extrapolate: any raw Pos difference counts, and +// equal positions stay silent. +func TestLocalStateChangedPausedPosition(t *testing.T) { + now := time.Now().UnixMilli() + last := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Paused", IsPlaying: false, Title: "T", Pos: 10000, PosAnchorMs: now - 60000} + seeked := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Paused", IsPlaying: false, Title: "T", Pos: 30000} + if !localStateChanged(now, seeked, last) { + t.Fatal("paused seek must count as changed") + } + same := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Paused", IsPlaying: false, Title: "T", Pos: 10000} + if localStateChanged(now, same, last) { + t.Fatal("unchanged paused position must not count as changed") + } +} + // Anchor is stamped on the broadcast state itself. func TestBroadcastAnchorFreshness(t *testing.T) { p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) diff --git a/internal/plugins/mpris/reconcile.go b/internal/plugins/mpris/reconcile.go index 24cc75c..832baaa 100644 --- a/internal/plugins/mpris/reconcile.go +++ b/internal/plugins/mpris/reconcile.go @@ -1,6 +1,8 @@ package mpris import ( + "time" + "github.com/bethropolis/kcd/internal/log" "github.com/godbus/dbus/v5" ) @@ -131,19 +133,32 @@ func (p *MPRISPlugin) refreshTrackedStates() { if err != nil { continue } + // The comparison reads the cached anchor, which broadcast + // stamps under p.mu — hold RLock through it. p.mu.RLock() - last := p.lastStates[display] + changed := localStateChanged(time.Now().UnixMilli(), state, p.lastStates[display]) p.mu.RUnlock() - if localStateChanged(state, last) { + if changed { p.storeLocalState(display, state) p.broadcast(state) } } } +// positionDriftToleranceMs bounds how far the phone's extrapolated +// position may drift from the true position before a poll tick +// re-broadcasts. Steady playback inside the tolerance stays silent (the +// phone extrapolates Pos + (now - PosAnchorMs) itself); seeks, missed +// signals, and clock drift beyond it refresh the anchor. +const positionDriftToleranceMs = 3000 + // localStateChanged reports whether a fresh read differs from the cached // state on any broadcasted field. A nil cache always counts as changed. -func localStateChanged(state, last *NowPlaying) bool { +// Position counts only on drift: while both reads agree the player is +// playing, the phone extrapolates from the anchor, so a tick that merely +// advances Pos on schedule is not a change. nowMs is the tick time the +// extrapolation is measured against. +func localStateChanged(nowMs int64, state, last *NowPlaying) bool { return last == nil || state.PlaybackStatus != last.PlaybackStatus || state.Title != last.Title || @@ -151,5 +166,22 @@ func localStateChanged(state, last *NowPlaying) bool { state.Album != last.Album || state.AlbumArtUrl != last.AlbumArtUrl || state.Volume != last.Volume || - state.IsPlaying != last.IsPlaying + state.IsPlaying != last.IsPlaying || + positionDrifted(nowMs, state, last) +} + +// positionDrifted reports whether the fresh position has moved off the +// phone's extrapolation past the tolerance. Outside steady playback +// (either side paused, or no anchor stamped yet) there is no +// extrapolation, so any raw Pos difference counts. +func positionDrifted(nowMs int64, state, last *NowPlaying) bool { + if state.IsPlaying && last.IsPlaying && last.PosAnchorMs > 0 { + expected := last.Pos + (nowMs - last.PosAnchorMs) + drift := state.Pos - expected + if drift < 0 { + drift = -drift + } + return drift > positionDriftToleranceMs + } + return state.Pos != last.Pos } diff --git a/internal/plugins/mpris/remote.go b/internal/plugins/mpris/remote.go index b270eab..ff1d207 100644 --- a/internal/plugins/mpris/remote.go +++ b/internal/plugins/mpris/remote.go @@ -192,26 +192,49 @@ func (p *MPRISPlugin) requestPlayerListPeriodic(dev device.Sender) { p.requestPlayerList(dev) } -// startRemoteStatePoller periodically re-requests now-playing from every +// syncRemotePoller starts the remote-state poller when at least one +// subscriber listens for mpris.update and stops it when the audience +// drains. Invoked from the bus subscriber-change hook (which runs without +// the bus lock) and once at construction. The hook only manages the +// ticker lifecycle; pollRemoteStates keeps its own guard so a racing +// unsubscribe between ticks still sends nothing. +func (p *MPRISPlugin) syncRemotePoller() { + p.mu.Lock() + defer p.mu.Unlock() + if p.bus.HasSubscribers(events.TypeMprisUpdate) { + if p.remotePollCancel == nil { + ctx, cancel := context.WithCancel(p.watchCtx) + p.remotePollCancel = cancel + go p.runRemoteStatePoller(ctx) + } + return + } + if p.remotePollCancel != nil { + p.remotePollCancel() + p.remotePollCancel = nil + } +} + +// runRemoteStatePoller periodically re-requests now-playing from every // connected device that has a known active player, but only while somebody -// listens: pollRemoteStates stays silent with zero mpris.update subscribers. +// listens: the ticker itself exists only with mpris.update subscribers +// (see syncRemotePoller), and pollRemoteStates stays silent with zero +// subscribers even if a tick races an unsubscribe. // The responses flow back through Handle, where shouldPublishRemoteState // dedupes them, so an mpris.update is only republished when the state // actually changes — not on every poll. This closes the "watch client // misses mid-track state" gap from the initial dump's 10s freshness gate. -func (p *MPRISPlugin) startRemoteStatePoller(ctx context.Context) { - go func() { - ticker := time.NewTicker(remoteStatePollInterval) - defer ticker.Stop() - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - p.pollRemoteStates() - } +func (p *MPRISPlugin) runRemoteStatePoller(ctx context.Context) { + ticker := time.NewTicker(remoteStatePollInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + p.pollRemoteStates() } - }() + } } // pollRemoteStates requests a now-playing refresh from devices that have a diff --git a/internal/plugins/mpris/remote_gate_test.go b/internal/plugins/mpris/remote_gate_test.go index 86c2965..bb411e8 100644 --- a/internal/plugins/mpris/remote_gate_test.go +++ b/internal/plugins/mpris/remote_gate_test.go @@ -59,6 +59,36 @@ func TestPollRemoteRefreshesWhileWatched(t *testing.T) { } } +// The ticker itself is demand-driven: with zero subscribers no poller +// goroutine exists, the first mpris.update subscribe starts it, and the +// last unsubscribe stops it. The hook fires synchronously inside +// Subscribe/Close, so no waiting is needed. +func TestRemotePollerFollowsSubscribers(t *testing.T) { + bus := events.NewBus(log.Nop()) + p := NewMPRISPlugin(nil, bus, false, config.MPRISConfig{}, log.Nop()) + if p.watchCancel != nil { + defer p.watchCancel() + } + + polling := func() bool { + p.mu.RLock() + defer p.mu.RUnlock() + return p.remotePollCancel != nil + } + + if polling() { + t.Fatal("remote poller running with zero subscribers") + } + sub := bus.Subscribe(4, events.TypeMprisUpdate) + if !polling() { + t.Fatal("remote poller not started on first subscribe") + } + sub.Close() + if polling() { + t.Fatal("remote poller still running after last unsubscribe") + } +} + // Unsubscribing silences the poller again — attach/detach cycles must not // leak refreshes. func TestPollRemoteSilentAfterUnsubscribe(t *testing.T) { From 8c55cd1fe2e7cd2fa492b1cd3261bce10158e16e Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:05:22 +0300 Subject: [PATCH 12/25] fix(justfile): stop clean from deleting tracked packaging sources `rm -rf packaging/` destroyed tracked files the release depends on: systemd units, shell completions, the example config, and the firewall profiles. Clean only generated output now. --- justfile | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/justfile b/justfile index 2396eaa..681084d 100644 --- a/justfile +++ b/justfile @@ -18,10 +18,11 @@ default: all test: CGO_ENABLED=0 go test -p 1 ./... -# Clean build artifacts +# Clean build artifacts. Only generated output: packaging/ holds tracked +# sources (systemd units, completions, example config) that goreleaser and +# the install scripts consume. @clean: - rm -f {{ bin_dir }}/{{ binary_daemon }} - rm -rf packaging/ + rm -rf {{ bin_dir }} dist/ echo "Cleanup complete" # Install the binary and systemd service From 5a632d24d3e70af407efc69ab3d62b5a0ce6bf67 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:05:38 +0300 Subject: [PATCH 13/25] fix(install): require the Go version the error message claims The message and go.mod both say 1.25, but the guard accepted 1.22. --- scripts/install.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/install.sh b/scripts/install.sh index ae08d3a..659abdb 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -75,7 +75,7 @@ command -v go >/dev/null 2>&1 \ GO_VERSION="$(go version | awk '{print $3}' | tr -d 'go')" REQUIRED_MAJOR=1 -REQUIRED_MINOR=22 +REQUIRED_MINOR=25 IFS='.' read -r MAJOR MINOR _ <<< "$GO_VERSION" if (( MAJOR < REQUIRED_MAJOR || (MAJOR == REQUIRED_MAJOR && MINOR < REQUIRED_MINOR) )); then die "Go ${GO_VERSION} is too old. kcd requires Go ${REQUIRED_MAJOR}.${REQUIRED_MINOR}+." From 61af97682299b43857cb81ce40031305af571048 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:05:56 +0300 Subject: [PATCH 14/25] fix(packaging): document the full firewall port set Only 1716/udp was listed, so a UFW user got working discovery and indefinitely hanging connections. Point at the kcd profile we already ship (1716/udp, 1716/tcp, 1739:1764/tcp) and say what each range is for. --- scripts/postinstall.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/postinstall.sh b/scripts/postinstall.sh index 0034736..657931d 100755 --- a/scripts/postinstall.sh +++ b/scripts/postinstall.sh @@ -16,8 +16,11 @@ echo "" echo "To enable the system-level service (per-user), run:" echo " sudo systemctl enable --now kcd@\$USER" echo "" -echo "If you use a firewall, allow the KDE Connect port:" -echo " UFW: sudo ufw allow 1716/udp" +echo "If you use a firewall, allow KDE Connect. Discovery alone is not" +echo "enough — without TCP the pair never connects, and without the" +echo "side-channel range file and clipboard transfers fail:" +echo " UFW: sudo ufw allow kcd" +echo " (1716/udp discovery, 1716/tcp control, 1739:1764/tcp transfers)" echo " Firewalld: sudo firewall-cmd --permanent --add-service=kcd" echo " sudo firewall-cmd --reload" echo "=================================================================" From 82d068407efacfcfe1250156a89530821cbaa023 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:12:22 +0300 Subject: [PATCH 15/25] feat(pair): return the verification code to the pairing client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RequestPairing already derived the out-of-band code and logged it, but only the daemon log saw it. A user running `kcd pair ` got "Pair request sent" and no code, so the check the code exists for — the user confirming the phone is really the phone — was impossible to perform from this side. RequestPairing now returns the code, handlePair ships it as PairResult, and the CLI prints it with the mismatch warning. Empty on the two paths that have no code of ours to show: already paired, and accepting a request the peer initiated (listen mode already printed that one). Integration tests cover both directions: an outbound request yields an 8-hex-character key, an inbound accept yields none. --- cmd/kcd/cli_pair.go | 11 +++- docs/CLI.md | 2 + docs/IPC_PROTOCOL.md | 12 +++- internal/daemon/daemon.go | 4 +- internal/integration/battery_test.go | 2 +- internal/integration/dedup_test.go | 2 +- internal/integration/pair_test.go | 95 +++++++++++++++++++++++++++- internal/ipc/handler.go | 9 ++- internal/ipc/ipc_test.go | 4 +- internal/ipc/proto.go | 8 +++ internal/plugins/pair/actions.go | 28 ++++---- pkg/client/client_devices.go | 18 ++++-- 12 files changed, 167 insertions(+), 28 deletions(-) diff --git a/cmd/kcd/cli_pair.go b/cmd/kcd/cli_pair.go index 10783be..d8acc77 100644 --- a/cmd/kcd/cli_pair.go +++ b/cmd/kcd/cli_pair.go @@ -45,10 +45,15 @@ Without a device ID: enter listen mode to receive and verify incoming pairing re if c.NArg() >= 1 { targetID := c.Args().First() - if err := cl.Pair(targetID); err != nil { + verificationKey, err := cl.Pair(targetID) + if err != nil { return err } fmt.Printf("Pair request sent / accepted for %s\n", targetID) + if verificationKey != "" { + fmt.Printf("Verification code: %s\n", verificationKey) + fmt.Println("Compare it with the code shown on the device. If they differ, cancel and unpair.") + } return nil } @@ -120,7 +125,7 @@ Without a device ID: enter listen mode to receive and verify incoming pairing re fmt.Println("Still listening… (Ctrl+C to cancel)") continue } - if err := cl.Pair(r.result.DeviceID); err != nil { + if _, err := cl.Pair(r.result.DeviceID); err != nil { return fmt.Errorf("failed to accept pairing: %w", err) } fmt.Printf("Paired with %s (%s)\n", protocol.DisplayName(r.result.DeviceName), r.result.DeviceID) @@ -139,7 +144,7 @@ Without a device ID: enter listen mode to receive and verify incoming pairing re _ = cl.Unpair(r.result.DeviceID) return nil } - if err := cl.Pair(r.result.DeviceID); err != nil { + if _, err := cl.Pair(r.result.DeviceID); err != nil { return fmt.Errorf("failed to accept pairing: %w", err) } fmt.Printf("Paired with %s (%s)\n", protocol.DisplayName(r.result.DeviceName), r.result.DeviceID) diff --git a/docs/CLI.md b/docs/CLI.md index fcc8680..7952822 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -236,6 +236,8 @@ kcd pair If the device has already sent a pair request to `kcd` (state `PairRequestedByPeer`), this accepts it. Otherwise, it connects to the device on demand (using its last-seen discovery address) and sends a new pair request — accept on your phone. +When `kcd` sends the request, it prints a **verification code**. Compare it with the code on the phone's prompt: they must match. A mismatch means something is intercepting the connection — cancel and `kcd unpair` the device. No code is printed when the phone initiated the request (use [listen mode](#listen-mode-headless--server) for that direction) or when the device is already paired. + ### Listen mode (headless / server) ```bash diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index f16d1db..439a362 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -125,7 +125,17 @@ Optional fields: using its last-seen discovery address (background auto-dial no longer connects to unpaired devices), then sends the pair request. -**Response data:** none (`{"ok": true}`) +**Response data:** `PairResult` + +```json +{"ok": true, "data": {"verificationKey": "A1B2C3D4"}} +``` + +`verificationKey` is the out-of-band code the peer displays so the user +can confirm the connection is not intercepted; `kcd pair ` +prints it for comparison. It is omitted when no request was sent — the +device was already paired, the peer had requested first (so the peer owns +the code), or the peer presented no certificate to derive it from. #### `pair_listen` diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index a895822..2c860a4 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -211,7 +211,9 @@ func Run(ctx context.Context, cfg *config.Config) error { logger.Warn("auto-accept on pair dial failed", log.Error(err)) } } else if dev.State() != device.StatePaired { - if err := pairPlugin.RequestPairing(dev); err != nil { + // No CLI is attached to this dial, so the verification code + // only reaches the log; RequestPairing already logs it. + if _, err := pairPlugin.RequestPairing(dev); err != nil { logger.Warn("pair request on dial failed", log.Error(err)) } } diff --git a/internal/integration/battery_test.go b/internal/integration/battery_test.go index 745f527..72589c9 100644 --- a/internal/integration/battery_test.go +++ b/internal/integration/battery_test.go @@ -94,7 +94,7 @@ func TestBatteryUpdateFlowIntegration(t *testing.T) { time.Sleep(100 * time.Millisecond) // Accept the pending pair request via IPC (auto_accept removed in v1.10) - if err := cl.Pair("mock-peer"); err != nil { + if _, err := cl.Pair("mock-peer"); err != nil { t.Fatalf("accept pair: %v", err) } time.Sleep(100 * time.Millisecond) diff --git a/internal/integration/dedup_test.go b/internal/integration/dedup_test.go index d183ea3..1ef1bb0 100644 --- a/internal/integration/dedup_test.go +++ b/internal/integration/dedup_test.go @@ -134,7 +134,7 @@ func TestDuplicateSessionFailoverIntegration(t *testing.T) { t.Fatalf("send pair: %v", err) } time.Sleep(100 * time.Millisecond) - if err := cl.Pair("mock-peer"); err != nil { + if _, err := cl.Pair("mock-peer"); err != nil { t.Fatalf("accept pair: %v", err) } if ev := nextFor(t, evCh, "mock-peer", "device.connected"); ev.Type != events.TypeDeviceConnected { diff --git a/internal/integration/pair_test.go b/internal/integration/pair_test.go index e82d44f..2617368 100644 --- a/internal/integration/pair_test.go +++ b/internal/integration/pair_test.go @@ -5,6 +5,7 @@ package integration import ( "context" "os" + "strings" "testing" "time" @@ -116,10 +117,15 @@ func TestPairFlowIntegration(t *testing.T) { // Give the daemon a moment to process the pair request time.Sleep(100 * time.Millisecond) - // Accept the pending pair request via IPC (auto_accept removed in v1.10) - if err := cl.Pair("mock-peer"); err != nil { + // Accept the pending pair request via IPC (auto_accept removed in v1.10). + // The peer initiated, so it owns the verification code and ours is empty. + verificationKey, err := cl.Pair("mock-peer") + if err != nil { t.Fatalf("accept pair: %v", err) } + if verificationKey != "" { + t.Errorf("accept path returned a verification key %q; the peer initiated", verificationKey) + } ev := nextDomainEvent(t, evCh, 3*time.Second) if ev.Type != events.TypePairAccepted { @@ -141,3 +147,88 @@ func TestPairFlowIntegration(t *testing.T) { t.Errorf("mock-peer not found in Paired state; got: %+v", devs) } } + +// Initiating pairing must hand the verification code back to the caller: +// the phone shows its own copy, and the user can only compare codes if +// this side displays one. The accept path returns empty, so the code +// proves it came from the outbound request. +func TestPairInitiateReturnsVerificationKeyIntegration(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_STATE_HOME", dir) + t.Setenv("XDG_CONFIG_HOME", dir) + + cfg := config.Defaults() + cfg.SocketPath = dir + "/kcd.sock" + cfg.CertFile = dir + "/cert.pem" + cfg.KeyFile = dir + "/key.pem" + cfg.DeviceID = "test-daemon-pair-init" + cfg.LogLevel = "debug" + cfg.Plugins.Battery = false + cfg.Plugins.Notification = false + cfg.Plugins.Clipboard = false + cfg.Plugins.Share = false + cfg.Plugins.RunCommand = false + cfg.Plugins.MPRIS = false + cfg.Plugins.Ping = false + cfg.Plugins.Telephony = false + cfg.Plugins.Connectivity = false + cfg.Plugins.Mousepad = false + cfg.Plugins.SFTP = false + cfg.Plugins.FindMyPhone = false + cfg.Plugins.LockDevice = false + cfg.Plugins.SystemVolume = false + cfg.Plugins.SMS = false + + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatal(err) + } + + _, cl := testutil.StartTestDaemon(t, cfg) + + peerCertPair, err := cert.LoadOrGenerate(dir+"/peer-cert.pem", dir+"/peer-key.pem", "mock-peer") + if err != nil { + t.Fatalf("peer cert: %v", err) + } + peer := testutil.NewMockPeer(t, cert.TLSConfig(peerCertPair)) + conn := peer.Dial("127.0.0.1:1716") + defer conn.Close() + + _, _ = peer.ReadPacket(conn) + + // Identity only — no pair request. The daemon must initiate, so the + // verification code is ours to report. + identPkt, _ := protocol.NewPacket(protocol.TypeIdentity, protocol.IdentityBody{ + DeviceID: "mock-peer", + DeviceName: "Mock Peer", + DeviceType: "phone", + ProtocolVersion: protocol.ProtocolVersion, + }) + if err := peer.SendPacket(conn, identPkt); err != nil { + t.Fatalf("send identity: %v", err) + } + + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + devs, err := cl.Devices() + if err != nil { + t.Fatalf("list devices: %v", err) + } + if len(devs) > 0 && devs[0].ID == "mock-peer" { + break + } + time.Sleep(20 * time.Millisecond) + } + + verificationKey, err := cl.Pair("mock-peer") + if err != nil { + t.Fatalf("initiate pair: %v", err) + } + if len(verificationKey) != 8 { + t.Fatalf("verification key %q: want 8 hex characters, got %d", verificationKey, len(verificationKey)) + } + for _, r := range verificationKey { + if !strings.ContainsRune("0123456789ABCDEF", r) { + t.Fatalf("verification key %q contains a non-hex character %q", verificationKey, r) + } + } +} diff --git a/internal/ipc/handler.go b/internal/ipc/handler.go index b3f817e..92d31b2 100644 --- a/internal/ipc/handler.go +++ b/internal/ipc/handler.go @@ -152,10 +152,15 @@ func (h *Handler) handlePair(payload []byte) Response { return Response{OK: false, Error: "failed to accept pairing: " + err.Error()} } } else { - // Initiate new pairing request - if err := h.pairPlugin.RequestPairing(dev); err != nil { + // Initiate new pairing request. The code lets the CLI show the + // user what to compare against the phone's prompt; without it + // the out-of-band check is impossible from this side. + verificationKey, err := h.pairPlugin.RequestPairing(dev) + if err != nil { return Response{OK: false, Error: "failed to request pairing: " + err.Error()} } + data, _ := json.Marshal(PairResult{VerificationKey: verificationKey}) + return Response{OK: true, Data: data} } return Response{OK: true} } diff --git a/internal/ipc/ipc_test.go b/internal/ipc/ipc_test.go index 594a0a0..66052fb 100644 --- a/internal/ipc/ipc_test.go +++ b/internal/ipc/ipc_test.go @@ -51,7 +51,7 @@ func TestIPCRoundTrip(t *testing.T) { } // Test 2: Pair (device exists) - if err := cl.Pair("dev123"); err != nil { + if _, err := cl.Pair("dev123"); err != nil { t.Errorf("Pair() failed: %v", err) } @@ -71,7 +71,7 @@ func TestIPCRoundTrip(t *testing.T) { } // Test 5: Pair (unknown device) - err = cl.Pair("unknown") + _, err = cl.Pair("unknown") if err == nil { t.Error("expected Pair() to fail for unknown device") } diff --git a/internal/ipc/proto.go b/internal/ipc/proto.go index 86062d6..5d52f94 100644 --- a/internal/ipc/proto.go +++ b/internal/ipc/proto.go @@ -82,6 +82,14 @@ type PairListenResult struct { Fingerprint string `json:"fingerprint,omitempty"` } +// PairResult is returned by CmdPair on success. VerificationKey is the +// out-of-band code the peer shows for the user to compare; it is empty +// when no request was sent (already paired, or a pending peer request +// that this accepted instead). +type PairResult struct { + VerificationKey string `json:"verificationKey,omitempty"` +} + // DevicePayload is sent in requests requiring a device ID (like pair/unpair/ping). type DevicePayload struct { DeviceID string `json:"deviceId"` diff --git a/internal/plugins/pair/actions.go b/internal/plugins/pair/actions.go index e389ced..5ebc02b 100644 --- a/internal/plugins/pair/actions.go +++ b/internal/plugins/pair/actions.go @@ -29,16 +29,20 @@ func (p *PairPlugin) AcceptPairing(dev *device.Device) error { return nil } -// RequestPairing initiates a pairing request to a device. -func (p *PairPlugin) RequestPairing(dev *device.Device) error { +// RequestPairing initiates a pairing request to a device and returns the +// out-of-band verification code the peer displays for the user to compare. +// The code is empty when nothing needed requesting: an already-paired +// device, or a pending request from the peer that this accepts instead +// (the peer owns the code in that direction). +func (p *PairPlugin) RequestPairing(dev *device.Device) (string, error) { if dev.State() == device.StatePaired { p.logger.Warn("device already paired", log.String("device_id", dev.ID())) - return nil + return "", nil } if dev.State() == device.StatePairRequestedByPeer { // They already requested, just accept - return p.AcceptPairing(dev) + return "", p.AcceptPairing(dev) } // The request timestamp seeds the verification code on both sides, @@ -46,7 +50,7 @@ func (p *PairPlugin) RequestPairing(dev *device.Device) error { timestamp := time.Now().Unix() pkt, err := protocol.NewPairPacket(protocol.PairAccept, timestamp) if err != nil { - return err + return "", err } p.mu.Lock() @@ -55,15 +59,17 @@ func (p *PairPlugin) RequestPairing(dev *device.Device) error { if err := dev.Send(pkt); err != nil { p.logger.Error("failed to send pair request", log.Error(err)) - return err + return "", err } - peerCert := dev.PeerCert() - if peerCert != nil { - vKey := cert.VerificationKey(p.localCert, peerCert, timestamp) + // Without a peer certificate (unauthenticated legacy device) there is + // no code to compare; the caller surfaces an empty string. + var verificationKey string + if peerCert := dev.PeerCert(); peerCert != nil { + verificationKey = cert.VerificationKey(p.localCert, peerCert, timestamp) p.logger.Info("pairing verification code", log.String("device_id", dev.ID()), - log.String("code", vKey)) + log.String("code", verificationKey)) } dev.SetState(device.StatePairRequested) @@ -73,7 +79,7 @@ func (p *PairPlugin) RequestPairing(dev *device.Device) error { p.onStateChanged() } - return nil + return verificationKey, nil } // RejectPairing rejects an incoming pair request. diff --git a/pkg/client/client_devices.go b/pkg/client/client_devices.go index d0e04eb..4aca43b 100644 --- a/pkg/client/client_devices.go +++ b/pkg/client/client_devices.go @@ -50,10 +50,20 @@ func (c *Client) PairListen() (*ipc.PairListenResult, error) { return &result, nil } -// Pair requests the daemon to pair with a specific device. -func (c *Client) Pair(deviceID string) error { - _, err := c.Call(ipc.CmdPair, ipc.DevicePayload{DeviceID: deviceID}) - return err +// Pair requests the daemon to pair with a specific device and returns the +// out-of-band verification code to compare against the peer's prompt. The +// code is empty when the device was already paired or had a pending +// request that the daemon accepted instead. +func (c *Client) Pair(deviceID string) (string, error) { + resp, err := c.Call(ipc.CmdPair, ipc.DevicePayload{DeviceID: deviceID}) + if err != nil { + return "", err + } + var result ipc.PairResult + if len(resp.Data) > 0 { + _ = json.Unmarshal(resp.Data, &result) + } + return result.VerificationKey, nil } // Unpair requests the daemon to unpair and forget a specific device. From 3715634cc83df9cbe3f84a5224dca11a57a86c95 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:19:25 +0300 Subject: [PATCH 16/25] fix(runcommand): make kcd run list return the command list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The command was dead on arrival. CmdRunList sent the request and returned OK immediately, but the plugin only registered kdeconnect.runcommand.request as an incoming type — so the phone's kdeconnect.runcommand reply hit "unhandled packet type" and was dropped. Nothing could ever consume the result, and the CLI's "run kcd watch to see results" pointed at a runcommand.list event that does not exist. The plugin now claims the reply type and hands the decoded list to a per-device waiter registered before the request goes out, so a fast phone cannot answer into a void. CmdRunList blocks on that waiter with the plugin's 10s deadline — the same shape as the sftp mount route — and returns the list; the CLI prints namecommand. Delivery to a waiter is non-blocking, so a late or unsolicited list cannot stall the device's read loop, and a second concurrent request for the same device fails fast rather than racing for the single reply. --- cmd/kcd/cli_run.go | 11 +- docs/CLI.md | 13 +- docs/IPC_PROTOCOL.md | 14 +- internal/daemon/ipc_routes_runcommand.go | 19 +- internal/plugins/runcommand/list.go | 144 +++++++++++++++ internal/plugins/runcommand/list_test.go | 208 ++++++++++++++++++++++ internal/plugins/runcommand/runcommand.go | 19 +- pkg/client/client_actions.go | 23 ++- 8 files changed, 431 insertions(+), 20 deletions(-) create mode 100644 internal/plugins/runcommand/list.go create mode 100644 internal/plugins/runcommand/list_test.go diff --git a/cmd/kcd/cli_run.go b/cmd/kcd/cli_run.go index e12b30a..3a575f4 100644 --- a/cmd/kcd/cli_run.go +++ b/cmd/kcd/cli_run.go @@ -22,10 +22,17 @@ var runCmd = &cli.Command{ if err != nil { return err } - if err := cl.RunList(c.Args().First()); err != nil { + commands, err := cl.RunList(c.Args().First()) + if err != nil { return err } - fmt.Println("Command list requested. Run 'kcd watch' to see results.") + if len(commands) == 0 { + fmt.Println("No commands available on this device.") + return nil + } + for _, cmd := range commands { + fmt.Printf("%s\t%s\n", cmd.Name, cmd.Command) + } return nil }, }, diff --git a/docs/CLI.md b/docs/CLI.md index 7952822..67900d3 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -782,12 +782,19 @@ Request the list of commands available on the remote device. kcd run list ``` -Results arrive as a `runcommand.list` event; watch for them: +The phone holds the list, so the command blocks until it replies (10s +timeout) and then prints `namecommand` per entry: -```bash -kcd watch --json | jq 'select(.type=="runcommand.list")' +```text +Take photo camera +Toggle flashlight light --toggle ``` +**Error cases:** the device is disconnected (`device not found`); the +`runcommand` plugin is disabled in `kcd.toml`; or the phone's KDE Connect +app is closed and does not answer within 10s. The KDE Connect app must be +open on the device for this to return anything. + ### run exec Execute one of the local commands registered in `[commands]` config, triggered from the phone, or send a command execution request to the phone. diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index 439a362..6700f9f 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -500,8 +500,17 @@ Request a device's list of configured run commands. {"deviceId": "a1b2c3d4e5f6_..."} ``` -**Response data:** none (results arrive via `kdeconnect.runcommand` response -packet). +**Response data:** `[]RemoteCommand` + +```json +{"ok": true, "data": [{"name": "Take photo", "command": "camera"}]} +``` + +The list lives only on the device, so the daemon holds this request open +until the device replies or 10s elapses. Errors: `device not found`, +`runcommand plugin not enabled`, a timeout naming the app that must be +open, or `a command list request for is already in flight` when two +clients race for the single reply. #### `run_exec` @@ -1380,6 +1389,7 @@ plugin processes it and a link to the body struct definition. | `kdeconnect.lock.request` | LockDevice | `LockBody{}` (triggers lock/unlock) | | `kdeconnect.mpris` | MPRIS | `MPRISRequest{RequestPlayerList, RequestNowPlaying, RequestVolume, Player, Action, AlbumArtUrl, TransferringAlbumArt, ...}` — inbound packets with `transferringAlbumArt: true` + `payloadTransferInfo` carry album art bytes (side channel) that the daemon caches to `$XDG_CACHE_HOME/kcd/art/` | | `kdeconnect.mpris.request` | MPRIS | `MPRISRequest{}` (same struct, different semantics) — an outbound `kdeconnect.mpris.request` with `player` + `albumArtUrl` asks the phone to stream art back | +| `kdeconnect.runcommand` | RunCommand | `{CommandList string}` — the phone's reply to a command-list request, holding a JSON object of label → `{name, command}` | | `kdeconnect.runcommand.request` | RunCommand | `RequestBody{RequestCommandList bool, Key string}` | | `kdeconnect.presenter` | Presenter | `PresenterBody{Dx, Dy *float64, Stop *bool}` | | `kdeconnect.systemvolume` | RemoteSystemVolume | `VolumeBody{SinkList, Name, Volume, Muted}` | diff --git a/internal/daemon/ipc_routes_runcommand.go b/internal/daemon/ipc_routes_runcommand.go index 88aa04c..d15cfcf 100644 --- a/internal/daemon/ipc_routes_runcommand.go +++ b/internal/daemon/ipc_routes_runcommand.go @@ -1,21 +1,30 @@ package daemon import ( + "context" + "github.com/bethropolis/kcd/internal/device" "github.com/bethropolis/kcd/internal/ipc" "github.com/bethropolis/kcd/internal/plugin" + "github.com/bethropolis/kcd/internal/plugins/runcommand" "github.com/bethropolis/kcd/internal/protocol" ) func registerRunCommandRoutes(handler *ipc.Handler, devices *device.Registry, plugins *plugin.Registry) { handler.Register(ipc.CmdRunList, func(req ipc.Request) ipc.Response { var p ipc.DevicePayload - return deviceRoute(req, &p, devices, plugins, "", func(dev *device.Device, _ plugin.Plugin) ipc.Response { - pkt, _ := protocol.NewPacket("kdeconnect.runcommand.request", map[string]bool{"requestCommandList": true}) - if err := dev.Send(pkt); err != nil { - return ipc.Response{OK: false, Error: "failed to send runcommand list request"} + // The list only exists on the phone, so this blocks until it + // answers or the plugin's own deadline expires — same shape as + // the sftp mount route. + return deviceRoute(req, &p, devices, plugins, "RunCommand", func(dev *device.Device, pl plugin.Plugin) ipc.Response { + commands, err := pl.(*runcommand.RunCommandPlugin).RequestList(context.Background(), dev) + if err != nil { + return ipc.Response{OK: false, Error: err.Error()} } - return ipc.Response{OK: true} + if commands == nil { + commands = []runcommand.Command{} + } + return jsonOK(commands) }) }) handler.Register(ipc.CmdRunExec, func(req ipc.Request) ipc.Response { diff --git a/internal/plugins/runcommand/list.go b/internal/plugins/runcommand/list.go new file mode 100644 index 0000000..7c47d61 --- /dev/null +++ b/internal/plugins/runcommand/list.go @@ -0,0 +1,144 @@ +package runcommand + +import ( + "context" + "encoding/json" + "fmt" + "sort" + "time" + + "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/log" + "github.com/bethropolis/kcd/internal/protocol" +) + +// listTimeout bounds the wait for the phone's command-list reply. The +// phone answers from a packet handler, so a prompt reply is the norm; +// the deadline only exists so a closed app surfaces as an error instead +// of hanging the CLI. +const listTimeout = 10 * time.Second + +// Command is one remote-executable entry as advertised by the phone. +type Command struct { + Name string `json:"name"` + Command string `json:"command"` +} + +// commandEntry is the per-label object inside the wire commandList map. +type commandEntry struct { + Name string `json:"name"` + Command string `json:"command"` +} + +// RequestList asks the device for its command list and waits for the +// reply. The waiter is registered before the request is sent so a fast +// phone cannot answer into a void. +// +// It fails if the device is disconnected, the send fails, or the phone +// does not answer within listTimeout. +func (p *RunCommandPlugin) RequestList(ctx context.Context, dev device.Sender) ([]Command, error) { + waiter := make(chan []Command, 1) + + p.Mu.Lock() + if p.pendingLists == nil { + p.pendingLists = make(map[string]chan []Command) + } + // A second concurrent request for the same device would race for the + // single reply; fail loudly rather than silently dropping one. + if _, busy := p.pendingLists[dev.ID()]; busy { + p.Mu.Unlock() + return nil, fmt.Errorf("runcommand: a command list request for %s is already in flight", dev.ID()) + } + p.pendingLists[dev.ID()] = waiter + p.Mu.Unlock() + + defer p.clearListWaiter(dev.ID()) + + pkt, err := protocol.NewPacket("kdeconnect.runcommand.request", RequestBody{RequestCommandList: true}) + if err != nil { + return nil, fmt.Errorf("runcommand: build list request: %w", err) + } + if err := dev.Send(pkt); err != nil { + return nil, fmt.Errorf("runcommand: send list request: %w", err) + } + + deadline, cancel := context.WithTimeout(ctx, listTimeout) + defer cancel() + + select { + case commands := <-waiter: + return commands, nil + case <-deadline.Done(): + return nil, fmt.Errorf("runcommand: timed out after %s waiting for the command list — is the KDE Connect app open on the phone?", listTimeout) + } +} + +// clearListWaiter drops a registered waiter so a later request is not +// blocked by an abandoned one. +func (p *RunCommandPlugin) clearListWaiter(deviceID string) { + p.Mu.Lock() + defer p.Mu.Unlock() + delete(p.pendingLists, deviceID) +} + +// deliverCommandList hands a decoded reply to the waiter for deviceID. +// The send is non-blocking: a reply with no waiter (the user gave up, or +// the phone volunteered a list) must not stall the device's read loop. +func (p *RunCommandPlugin) deliverCommandList(deviceID string, commands []Command) { + p.Mu.RLock() + waiter := p.pendingLists[deviceID] + p.Mu.RUnlock() + + if waiter == nil { + p.logger.Debug("runcommand: command list with no waiter, dropping", + log.String("device_id", deviceID), + log.Int("commands", len(commands))) + return + } + select { + case waiter <- commands: + default: + } +} + +// parseCommandList decodes the commandList field of a runcommand reply. +// The phone sends a JSON object of label -> {name, command}; malformed +// entries are skipped rather than failing the whole list. +func parseCommandList(raw string) ([]Command, error) { + if raw == "" { + return nil, nil + } + var entries map[string]commandEntry + if err := json.Unmarshal([]byte(raw), &entries); err != nil { + return nil, fmt.Errorf("runcommand: parse command list: %w", err) + } + + commands := make([]Command, 0, len(entries)) + for label, entry := range entries { + name := entry.Name + if name == "" { + name = label + } + commands = append(commands, Command{Name: name, Command: entry.Command}) + } + sort.Slice(commands, func(i, j int) bool { return commands[i].Name < commands[j].Name }) + return commands, nil +} + +// handleListReply processes a kdeconnect.runcommand packet carrying the +// peer's command list. +func (p *RunCommandPlugin) handleListReply(dev device.Sender, pkt *protocol.Packet) { + var body struct { + CommandList string `json:"commandList"` + } + if err := json.Unmarshal(pkt.Body, &body); err != nil { + p.logger.Warn("runcommand: malformed list reply", log.Error(err)) + return + } + commands, err := parseCommandList(body.CommandList) + if err != nil { + p.logger.Warn("runcommand: undecodable list reply", log.Error(err)) + return + } + p.deliverCommandList(dev.ID(), commands) +} diff --git a/internal/plugins/runcommand/list_test.go b/internal/plugins/runcommand/list_test.go new file mode 100644 index 0000000..ce22d78 --- /dev/null +++ b/internal/plugins/runcommand/list_test.go @@ -0,0 +1,208 @@ +package runcommand + +import ( + "context" + "crypto/x509" + "encoding/json" + "net" + "testing" + "time" + + "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/log" + "github.com/bethropolis/kcd/internal/protocol" +) + +// listSender is a device.Sender that answers a command-list request +// inline, the way a phone replies from its packet handler. Injected via +// onSend so the round trip stays in one goroutine-free call stack. +type listSender struct { + id string + onSend func(*protocol.Packet) + sent int +} + +func (s *listSender) ID() string { return s.id } +func (s *listSender) Name() string { return "Test" } +func (s *listSender) SetName(string) {} +func (s *listSender) State() device.PairingState { return device.StatePaired } +func (s *listSender) SetState(device.PairingState) {} +func (s *listSender) IsConnected() bool { return true } +func (s *listSender) RemoteIP() net.IP { return nil } +func (s *listSender) PeerCert() *x509.Certificate { return nil } +func (s *listSender) HasCapability(string) bool { return false } +func (s *listSender) UpdateBattery(int, bool) {} +func (s *listSender) GetBattery() (int, bool) { return 0, false } +func (s *listSender) Send(p *protocol.Packet) error { + s.sent++ + if s.onSend != nil { + s.onSend(p) + } + return nil +} + +// replyWith builds a runcommand reply packet carrying commandList. +func replyWith(t *testing.T, commandList string) *protocol.Packet { + t.Helper() + pkt, err := protocol.NewPacket("kdeconnect.runcommand", map[string]string{"commandList": commandList}) + if err != nil { + t.Fatal(err) + } + return pkt +} + +func TestRequestListReturnsDeviceCommands(t *testing.T) { + logger := log.NewTest(t) + p := NewRunCommandPlugin(nil, nil, logger) + + sender := &listSender{id: "dev1"} + sender.onSend = func(pkt *protocol.Packet) { + // A phone answers the request it just received. + if err := p.Handle(context.Background(), sender, replyWith(t, + `{"take-photo":{"name":"Take photo","command":"camera"},"lock":{"name":"Lock","command":"x"}}`)); err != nil { + t.Errorf("reply Handle failed: %v", err) + } + } + + commands, err := p.RequestList(context.Background(), sender) + if err != nil { + t.Fatalf("RequestList failed: %v", err) + } + if len(commands) != 2 { + t.Fatalf("got %d commands, want 2: %+v", len(commands), commands) + } + // Sorted by name so the CLI output is stable across replies. + if commands[0].Name != "Lock" || commands[1].Name != "Take photo" { + t.Errorf("commands not sorted by name: %+v", commands) + } + if commands[1].Command != "camera" { + t.Errorf("command payload lost: %+v", commands[1]) + } + if sender.sent != 1 { + t.Errorf("sent %d packets, want exactly 1 request", sender.sent) + } +} + +// The waiter must be released whether the phone answers or not, or a +// second `kcd run list` would be rejected as "already in flight" forever. +func TestRequestListReleasesWaiterAfterReply(t *testing.T) { + logger := log.NewTest(t) + p := NewRunCommandPlugin(nil, nil, logger) + + sender := &listSender{id: "dev1"} + sender.onSend = func(pkt *protocol.Packet) { + _ = p.Handle(context.Background(), sender, replyWith(t, `{"a":{"name":"A","command":"a"}}`)) + } + + for i := range 2 { + if _, err := p.RequestList(context.Background(), sender); err != nil { + t.Fatalf("RequestList call %d failed: %v", i, err) + } + } + + p.Mu.RLock() + pending := len(p.pendingLists) + p.Mu.RUnlock() + if pending != 0 { + t.Errorf("%d waiters left registered after completion, want 0", pending) + } +} + +// A reply that arrives with nobody waiting must not block the device's +// read loop or crash on a nil channel. +func TestHandleListReplyWithoutWaiterDoesNotBlock(t *testing.T) { + logger := log.NewTest(t) + p := NewRunCommandPlugin(nil, nil, logger) + sender := &listSender{id: "dev1"} + + done := make(chan struct{}) + go func() { + defer close(done) + if err := p.Handle(context.Background(), sender, replyWith(t, `{"a":{"name":"A","command":"a"}}`)); err != nil { + t.Errorf("Handle failed: %v", err) + } + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Handle blocked on a reply with no waiter") + } +} + +// A malformed list must not wedge the waiter or panic. +func TestHandleListReplyMalformedIsDropped(t *testing.T) { + logger := log.NewTest(t) + p := NewRunCommandPlugin(nil, nil, logger) + sender := &listSender{id: "dev1"} + + if err := p.Handle(context.Background(), sender, replyWith(t, `not json`)); err != nil { + t.Errorf("Handle should swallow a malformed list, got %v", err) + } +} + +// Empty commandList is a valid answer meaning "no commands". +func TestParseCommandListEmpty(t *testing.T) { + commands, err := parseCommandList("") + if err != nil { + t.Fatalf("parseCommandList(\"\") failed: %v", err) + } + if len(commands) != 0 { + t.Errorf("got %d commands from an empty list, want 0", len(commands)) + } +} + +// An entry without a name falls back to its map key, so a sparse phone +// implementation still lists something usable. +func TestParseCommandListFallsBackToKey(t *testing.T) { + var entries map[string]commandEntry + if err := json.Unmarshal([]byte(`{"screenshot":{"command":"shot"}}`), &entries); err != nil { + t.Fatal(err) + } + commands, err := parseCommandList(`{"screenshot":{"command":"shot"}}`) + if err != nil { + t.Fatalf("parseCommandList failed: %v", err) + } + if len(commands) != 1 || commands[0].Name != "screenshot" { + t.Errorf("key fallback failed: %+v", commands) + } + if commands[0].Command != "shot" { + t.Errorf("command lost: %+v", commands[0]) + } +} + +// A second concurrent request for the same device must fail fast rather +// than race for the single reply. +func TestRequestListRejectsConcurrentRequest(t *testing.T) { + logger := log.NewTest(t) + p := NewRunCommandPlugin(nil, nil, logger) + + // No onSend: the first request parks until its context is cancelled. + sender := &listSender{id: "dev1"} + + firstCtx, cancelFirst := context.WithCancel(context.Background()) + defer cancelFirst() + errCh := make(chan error, 1) + go func() { + _, err := p.RequestList(firstCtx, sender) + errCh <- err + }() + + // Give the first request time to register its waiter. + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + p.Mu.RLock() + _, busy := p.pendingLists["dev1"] + p.Mu.RUnlock() + if busy { + break + } + time.Sleep(5 * time.Millisecond) + } + + if _, err := p.RequestList(context.Background(), sender); err == nil { + t.Fatal("second concurrent RequestList should fail, got nil error") + } + cancelFirst() + <-errCh +} diff --git a/internal/plugins/runcommand/runcommand.go b/internal/plugins/runcommand/runcommand.go index 3c2f206..628e2fe 100644 --- a/internal/plugins/runcommand/runcommand.go +++ b/internal/plugins/runcommand/runcommand.go @@ -19,8 +19,11 @@ type RunCommandPlugin struct { Mu sync.RWMutex // exported so daemon.go can lock it during reload Commands map[string]string CommandsPerDevice map[string]map[string]string // keyed by device ID - logger log.Logger - wg sync.WaitGroup // exported for tests to synchronize with background goroutines + // pendingLists holds one buffered waiter per device awaiting that + // device's command-list reply, keyed by device ID. + pendingLists map[string]chan []Command + logger log.Logger + wg sync.WaitGroup // exported for tests to synchronize with background goroutines } func NewRunCommandPlugin(commands map[string]string, commandsPerDevice map[string]map[string]string, logger log.Logger) *RunCommandPlugin { @@ -30,6 +33,7 @@ func NewRunCommandPlugin(commands map[string]string, commandsPerDevice map[strin return &RunCommandPlugin{ Commands: commands, CommandsPerDevice: commandsPerDevice, + pendingLists: make(map[string]chan []Command), logger: logger.With(log.String("plugin", "runcommand")), } } @@ -46,9 +50,11 @@ func (p *RunCommandPlugin) Name() string { return "RunCommand" } // Timeout returns the timeout. func (p *RunCommandPlugin) Timeout() time.Duration { return 5 * time.Second } -// IncomingTypes returns the packet types this plugin handles. +// IncomingTypes returns the packet types this plugin handles. The bare +// kdeconnect.runcommand type carries the phone's reply to a command-list +// request; without it the reply is dropped as unhandled. func (p *RunCommandPlugin) IncomingTypes() []string { - return []string{"kdeconnect.runcommand.request"} + return []string{"kdeconnect.runcommand.request", "kdeconnect.runcommand"} } // OutgoingTypes returns the packet types this plugin may send. @@ -58,6 +64,11 @@ func (p *RunCommandPlugin) OutgoingTypes() []string { // Handle processes incoming command requests. func (p *RunCommandPlugin) Handle(ctx context.Context, dev device.Sender, pkt *protocol.Packet) error { + if pkt.Type == "kdeconnect.runcommand" { + p.handleListReply(dev, pkt) + return nil + } + var body RequestBody if err := json.Unmarshal(pkt.Body, &body); err != nil { return err diff --git a/pkg/client/client_actions.go b/pkg/client/client_actions.go index d73e14a..a8cc5b1 100644 --- a/pkg/client/client_actions.go +++ b/pkg/client/client_actions.go @@ -35,10 +35,25 @@ func (c *Client) ClipboardPush(deviceID string) error { return err } -// RunList requests the remote device to send its command list. -func (c *Client) RunList(deviceID string) error { - _, err := c.Call(ipc.CmdRunList, ipc.DevicePayload{DeviceID: deviceID}) - return err +// RemoteCommand is one executable command advertised by a remote device. +type RemoteCommand struct { + Name string `json:"name"` + Command string `json:"command"` +} + +// RunList asks the remote device for its command list and returns it. The +// daemon holds the request open until the device replies, so this blocks +// for the reply or fails on timeout. +func (c *Client) RunList(deviceID string) ([]RemoteCommand, error) { + resp, err := c.Call(ipc.CmdRunList, ipc.DevicePayload{DeviceID: deviceID}) + if err != nil { + return nil, err + } + var commands []RemoteCommand + if len(resp.Data) > 0 { + _ = json.Unmarshal(resp.Data, &commands) + } + return commands, nil } // RunExec requests the remote device to execute a specific command key. From 617f4d3b882b19f39e0797563c7e83a66e7b5830 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:22:02 +0300 Subject: [PATCH 17/25] fix(mpris): don't force playback when skipping a paused player MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit next/previous sent the skip and then unconditionally sent Play, so skipping a paused player unpaused it and started audio the user never asked for. The Play nudge exists for phones whose MPRIS implementation stops after a track change, and that is only needed when something was playing to begin with. Both commands now read the player's state first and nudge only if it was playing. State the daemon cannot report — query failure, unknown device, no matching player — defaults to nudging, preserving the workaround for the phones that need it instead of silently dropping it. --- cmd/kcd/cli_mpris.go | 82 ++++++++++++++++++++++++++------------- cmd/kcd/cli_mpris_test.go | 80 ++++++++++++++++++++++++++++++++++++++ docs/CLI.md | 2 +- 3 files changed, 137 insertions(+), 27 deletions(-) create mode 100644 cmd/kcd/cli_mpris_test.go diff --git a/cmd/kcd/cli_mpris.go b/cmd/kcd/cli_mpris.go index e757cf1..3c9864c 100644 --- a/cmd/kcd/cli_mpris.go +++ b/cmd/kcd/cli_mpris.go @@ -6,6 +6,7 @@ import ( "os" "strconv" + "github.com/bethropolis/kcd/pkg/client" "github.com/urfave/cli/v2" ) @@ -24,6 +25,55 @@ func actionCmd(action string) cli.ActionFunc { } } +// skipCmd builds the next/previous actions. Some phone MPRIS +// implementations stop after a skip, so the new track is nudged with +// Play — but only when something was actually playing. Skipping a paused +// player must leave it paused; unpausing it would start audio the user +// never asked for. +func skipCmd(action string) cli.ActionFunc { + return func(c *cli.Context) error { + cl, err := getClient(c) + if err != nil { + return err + } + deviceID := c.String("device") + player := c.String("player") + wasPlaying := remotePlayerPlaying(cl, deviceID, player) + + if err := cl.MprisAction(deviceID, player, action); err != nil { + return err + } + if !wasPlaying { + return nil + } + return cl.MprisAction(deviceID, player, "Play") + } +} + +// remotePlayerPlaying reports whether the target player is currently +// playing. An empty player name means the device's only/first player. +// +// State the daemon cannot report — the query failed, the device is +// unknown, or no player matches — returns true, preserving the old +// always-nudge behavior for the phones that need it rather than +// silently dropping the workaround. +func remotePlayerPlaying(cl *client.Client, deviceID, player string) bool { + remote, err := cl.MprisRemote() + if err != nil { + return true + } + for _, p := range remote.Players { + if deviceID != "" && p.DeviceID != deviceID { + continue + } + if player != "" && p.Player != player { + continue + } + return p.IsPlaying + } + return true +} + var mprisCmd = &cli.Command{ Name: "mpris", Usage: "Media player control (remote devices)", @@ -161,37 +211,17 @@ var mprisCmd = &cli.Command{ Action: actionCmd("PlayPause"), }, { - Name: "next", - Usage: "Skip to next track on a remote device (also starts playback)", - Flags: actionFlags, - Action: func(c *cli.Context) error { - cl, err := getClient(c) - if err != nil { - return err - } - if err := cl.MprisAction(c.String("device"), c.String("player"), "Next"); err != nil { - return err - } - // Some phone MPRIS implementations stop after Next. - // Send Play to ensure the new track starts. - return cl.MprisAction(c.String("device"), c.String("player"), "Play") - }, + Name: "next", + Usage: "Skip to next track on a remote device (resumes playback if it was playing)", + Flags: actionFlags, + Action: skipCmd("Next"), }, { Name: "previous", Aliases: []string{"prev"}, - Usage: "Go to previous track on a remote device (also starts playback)", + Usage: "Go to previous track on a remote device (resumes playback if it was playing)", Flags: actionFlags, - Action: func(c *cli.Context) error { - cl, err := getClient(c) - if err != nil { - return err - } - if err := cl.MprisAction(c.String("device"), c.String("player"), "Previous"); err != nil { - return err - } - return cl.MprisAction(c.String("device"), c.String("player"), "Play") - }, + Action: skipCmd("Previous"), }, { Name: "stop", diff --git a/cmd/kcd/cli_mpris_test.go b/cmd/kcd/cli_mpris_test.go new file mode 100644 index 0000000..3fbef47 --- /dev/null +++ b/cmd/kcd/cli_mpris_test.go @@ -0,0 +1,80 @@ +package main + +import ( + "context" + "encoding/json" + "path/filepath" + "testing" + "time" + + "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/ipc" + "github.com/bethropolis/kcd/internal/log" + "github.com/bethropolis/kcd/internal/plugin" + "github.com/bethropolis/kcd/pkg/client" +) + +// startMprisStub runs an IPC server whose mpris remote route returns +// players, and returns a client wired to it. +func startMprisStub(t *testing.T, players []ipc.MprisRemotePlayer) *client.Client { + t.Helper() + sockPath := filepath.Join(t.TempDir(), "test.sock") + logger := log.NewTest(t) + + handler := ipc.NewHandler(device.NewRegistry(nil), plugin.NewRegistry(logger), nil, "", nil, 0) + handler.Register(ipc.CmdMprisRemote, func(ipc.Request) ipc.Response { + data, _ := json.Marshal(ipc.MprisRemoteResponse{Players: players}) + return ipc.Response{OK: true, Data: data} + }) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { _ = ipc.NewServer(sockPath, handler, logger).Listen(ctx) }() + time.Sleep(100 * time.Millisecond) + + return &client.Client{SocketPath: sockPath, Timeout: 2 * time.Second} +} + +// A paused player must report false so the skip leaves it paused; a +// playing one must report true so the skip can resume the next track. +func TestRemotePlayerPlaying(t *testing.T) { + players := []ipc.MprisRemotePlayer{ + {DeviceID: "dev1", Player: "Spotify", IsPlaying: true}, + {DeviceID: "dev2", Player: "Metrolist", IsPlaying: false}, + } + cl := startMprisStub(t, players) + + tests := []struct { + name string + deviceID string + player string + want bool + }{ + {"playing player", "dev1", "Spotify", true}, + {"paused player", "dev2", "Metrolist", false}, + {"empty player name takes the device's player", "dev2", "", false}, + {"empty device and player takes the first", "", "", true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := remotePlayerPlaying(cl, tc.deviceID, tc.player); got != tc.want { + t.Errorf("remotePlayerPlaying(%q, %q) = %v, want %v", tc.deviceID, tc.player, got, tc.want) + } + }) + } +} + +// State the daemon cannot report must default to true: dropping the Play +// nudge would regress the phones that stop after a skip. +func TestRemotePlayerPlayingUnknownDefaultsToNudge(t *testing.T) { + cl := startMprisStub(t, []ipc.MprisRemotePlayer{ + {DeviceID: "dev1", Player: "Spotify", IsPlaying: false}, + }) + + if !remotePlayerPlaying(cl, "dev1", "NoSuchPlayer") { + t.Error("an unmatched player must default to true (nudge), got false") + } + if !remotePlayerPlaying(cl, "unknown-device", "") { + t.Error("an unknown device must default to true (nudge), got false") + } +} diff --git a/docs/CLI.md b/docs/CLI.md index 67900d3..b6e9595 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -441,7 +441,7 @@ kcd mpris toggle [--device ] [--player ] ### mpris next / prev -Skip to the next or previous track. After skipping, an automatic `Play` action is sent to handle phone-side MPRIS implementations that stop after a track change. +Skip to the next or previous track. If the player was playing, a `Play` action follows the skip to handle phone-side MPRIS implementations that stop after a track change. A paused player is left paused — skipping never starts audio on its own. ``` kcd mpris next [--device ] [--player ] From 09d8aa644ec1926cae3f0e44d09f779659a96a29 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:33:35 +0300 Subject: [PATCH 18/25] refactor(cli): one device-resolution rule, and say what lock does Three commands (clipboard, connectivity, findmyphone) each carried their own copy of "first paired connected device, else error", while battery, ping, lock, unlock, volume, and contacts all demanded an explicit ID. The same setup therefore behaved differently depending on which command you reached for. The loop is now resolveDeviceID: an explicit argument wins, a lone paired connected device is selected, and several candidates is an error naming them rather than a silent pick against the wrong phone. Applied to every command whose only positional is the device. Commands with further positionals (volume set/mute) keep it mandatory so their remaining arguments stay unambiguous. mpris actions also take the device positionally now, matching the rest of the CLI; --device remains as a fallback and the positional wins. lock/unlock said "Lock the current desktop session" and the docs claimed they shell out to loginctl. Neither was true: the CLI sends the KDE Connect lock packet to the remote device. loginctl is the daemon's behavior when a phone asks *this* desktop to lock. --- cmd/kcd/cli_battery.go | 11 ++-- cmd/kcd/cli_clipboard.go | 22 +------ cmd/kcd/cli_connectivity.go | 19 +----- cmd/kcd/cli_contacts.go | 32 +++++----- cmd/kcd/cli_device.go | 44 ++++++++++++++ cmd/kcd/cli_device_test.go | 116 ++++++++++++++++++++++++++++++++++++ cmd/kcd/cli_findmyphone.go | 19 +----- cmd/kcd/cli_lock.go | 13 ++-- cmd/kcd/cli_mpris.go | 71 +++++++++++++--------- cmd/kcd/cli_ping.go | 11 ++-- cmd/kcd/cli_unlock.go | 13 ++-- cmd/kcd/cli_volume.go | 11 ++-- docs/CLI.md | 61 +++++++++++++------ 13 files changed, 304 insertions(+), 139 deletions(-) create mode 100644 cmd/kcd/cli_device.go create mode 100644 cmd/kcd/cli_device_test.go diff --git a/cmd/kcd/cli_battery.go b/cmd/kcd/cli_battery.go index 1158398..370e394 100644 --- a/cmd/kcd/cli_battery.go +++ b/cmd/kcd/cli_battery.go @@ -10,7 +10,7 @@ import ( var batteryCmd = &cli.Command{ Name: "battery", Usage: "Fetch battery level and charging status", - ArgsUsage: "", + ArgsUsage: "[device-id]", Flags: []cli.Flag{ &cli.BoolFlag{ Name: "json", @@ -18,14 +18,15 @@ var batteryCmd = &cli.Command{ }, }, Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - charge, charging, err := cl.Battery(c.Args().First()) + deviceID, err := resolveDeviceID(c, cl) + if err != nil { + return err + } + charge, charging, err := cl.Battery(deviceID) if err != nil { return err } diff --git a/cmd/kcd/cli_clipboard.go b/cmd/kcd/cli_clipboard.go index c171769..21d5576 100644 --- a/cmd/kcd/cli_clipboard.go +++ b/cmd/kcd/cli_clipboard.go @@ -6,7 +6,6 @@ import ( "os" "os/exec" - "github.com/bethropolis/kcd/internal/device" "github.com/urfave/cli/v2" ) @@ -31,25 +30,10 @@ var clipboardCmd = &cli.Command{ return err } - var targetID string - if c.NArg() >= 1 { - targetID = c.Args().First() - } else { - devs, err := cl.Devices() - if err != nil { - return err - } - for _, d := range devs { - if d.Connected && d.State == device.StatePaired { - targetID = d.ID - break - } - } - if targetID == "" { - return fmt.Errorf("no paired connected devices found") - } + targetID, err := resolveDeviceID(c, cl) + if err != nil { + return err } - if err := cl.ClipboardPush(targetID); err != nil { return err } diff --git a/cmd/kcd/cli_connectivity.go b/cmd/kcd/cli_connectivity.go index 290099d..1c068e5 100644 --- a/cmd/kcd/cli_connectivity.go +++ b/cmd/kcd/cli_connectivity.go @@ -6,7 +6,6 @@ import ( "sort" "strings" - "github.com/bethropolis/kcd/internal/device" "github.com/bethropolis/kcd/internal/plugins/connectivity" "github.com/urfave/cli/v2" ) @@ -27,21 +26,9 @@ var connectivityCmd = &cli.Command{ return err } - targetID := c.Args().First() - if targetID == "" { - devs, err := cl.Devices() - if err != nil { - return err - } - for _, d := range devs { - if d.Connected && d.State == device.StatePaired { - targetID = d.ID - break - } - } - if targetID == "" { - return fmt.Errorf("no paired connected devices found") - } + targetID, err := resolveDeviceID(c, cl) + if err != nil { + return err } raw, err := cl.Connectivity(targetID) diff --git a/cmd/kcd/cli_contacts.go b/cmd/kcd/cli_contacts.go index f5cda92..7dee3d1 100644 --- a/cmd/kcd/cli_contacts.go +++ b/cmd/kcd/cli_contacts.go @@ -15,16 +15,17 @@ var contactsCmd = &cli.Command{ { Name: "sync", Usage: "Request a contacts sync from a device", - ArgsUsage: "", + ArgsUsage: "[device-id]", Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - if err := cl.ContactsSync(c.Args().Get(0)); err != nil { + deviceID, err := resolveDeviceID(c, cl) + if err != nil { + return err + } + if err := cl.ContactsSync(deviceID); err != nil { return err } fmt.Println("Contacts sync requested. Use `kcd watch --events contacts.updated` to see results.") @@ -34,7 +35,7 @@ var contactsCmd = &cli.Command{ { Name: "list", Usage: "List cached contacts for a device", - ArgsUsage: "", + ArgsUsage: "[device-id]", Flags: []cli.Flag{ &cli.BoolFlag{ Name: "json", @@ -42,14 +43,15 @@ var contactsCmd = &cli.Command{ }, }, Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - list, err := cl.ContactsList(c.Args().Get(0)) + deviceID, err := resolveDeviceID(c, cl) + if err != nil { + return err + } + list, err := cl.ContactsList(deviceID) if err != nil { return err } @@ -72,16 +74,16 @@ var contactsCmd = &cli.Command{ { Name: "clear", Usage: "Delete cached contacts for a device (re-sync restores them)", - ArgsUsage: "", + ArgsUsage: "[device-id]", Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - id := c.Args().Get(0) + id, err := resolveDeviceID(c, cl) + if err != nil { + return err + } list, err := cl.ContactsList(id) if err != nil { return err diff --git a/cmd/kcd/cli_device.go b/cmd/kcd/cli_device.go new file mode 100644 index 0000000..7d48a1f --- /dev/null +++ b/cmd/kcd/cli_device.go @@ -0,0 +1,44 @@ +package main + +import ( + "fmt" + "sort" + "strings" + + "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/pkg/client" + "github.com/urfave/cli/v2" +) + +// resolveDeviceID returns the device a command should act on. +// +// An explicit argument always wins. Otherwise the sole paired, connected +// device is selected, so the common single-phone setup needs no ID. With +// several candidates it is an error naming them rather than a silent +// coin flip against the wrong phone. +func resolveDeviceID(c *cli.Context, cl *client.Client) (string, error) { + if c.NArg() >= 1 { + return c.Args().First(), nil + } + + devs, err := cl.Devices() + if err != nil { + return "", err + } + + var paired []string + for _, d := range devs { + if d.Connected && d.State == device.StatePaired { + paired = append(paired, d.ID) + } + } + switch len(paired) { + case 0: + return "", fmt.Errorf("no paired connected devices found — pass a device ID") + case 1: + return paired[0], nil + default: + sort.Strings(paired) + return "", fmt.Errorf("multiple devices connected (%s) — pass a device ID", strings.Join(paired, ", ")) + } +} diff --git a/cmd/kcd/cli_device_test.go b/cmd/kcd/cli_device_test.go new file mode 100644 index 0000000..1835c0a --- /dev/null +++ b/cmd/kcd/cli_device_test.go @@ -0,0 +1,116 @@ +package main + +import ( + "context" + "encoding/json" + "flag" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/ipc" + "github.com/bethropolis/kcd/internal/log" + "github.com/bethropolis/kcd/internal/plugin" + "github.com/bethropolis/kcd/pkg/client" + "github.com/urfave/cli/v2" +) + +// startDeviceStub runs an IPC server whose devices route returns a fixed +// device list, plus a client wired to it. +func startDeviceStub(t *testing.T, devs []device.DeviceInfo) *client.Client { + t.Helper() + sockPath := filepath.Join(t.TempDir(), "test.sock") + logger := log.NewTest(t) + + devReg := device.NewRegistry(nil) + for _, d := range devs { + devReg.Add(device.NewDevice(d.ID, d.Name, "phone", logger)) + } + + handler := ipc.NewHandler(devReg, plugin.NewRegistry(logger), nil, "", nil, 0) + handler.Register(ipc.CmdDevices, func(ipc.Request) ipc.Response { + data, _ := json.Marshal(devs) + return ipc.Response{OK: true, Data: data} + }) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { _ = ipc.NewServer(sockPath, handler, logger).Listen(ctx) }() + time.Sleep(100 * time.Millisecond) + + return &client.Client{SocketPath: sockPath, Timeout: 2 * time.Second} +} + +func TestResolveDeviceID(t *testing.T) { + paired := device.DeviceInfo{ID: "phone1", Name: "Phone", State: device.StatePaired, Connected: true} + other := device.DeviceInfo{ID: "phone2", Name: "Tablet", State: device.StatePaired, Connected: true} + unpaired := device.DeviceInfo{ID: "stranger", Name: "Stranger", State: device.StateUnpaired, Connected: true} + + tests := []struct { + name string + devices []device.DeviceInfo + want string + wantErr string + }{ + {"single paired device auto-resolves", []device.DeviceInfo{paired}, "phone1", ""}, + {"unpaired devices are ignored", []device.DeviceInfo{unpaired}, "", "no paired connected devices"}, + { + name: "multiple devices require an explicit ID", + devices: []device.DeviceInfo{paired, other}, + want: "", + // The error must name the candidates so the user can pick. + wantErr: "phone1, phone2", + }, + {"no devices errors", nil, "", "no paired connected devices"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cl := startDeviceStub(t, tc.devices) + set := flag.NewFlagSet("test", flag.ContinueOnError) + c := cli.NewContext(cli.NewApp(), set, nil) + + got, err := resolveDeviceID(c, cl) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil (resolved %q)", tc.wantErr, got) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("error %q does not contain %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tc.want { + t.Errorf("resolved %q, want %q", got, tc.want) + } + }) + } +} + +// An explicit positional always wins over auto-resolution, even when +// several devices are connected. +func TestResolveDeviceIDExplicitArgumentWins(t *testing.T) { + cl := startDeviceStub(t, []device.DeviceInfo{ + {ID: "phone1", State: device.StatePaired, Connected: true}, + {ID: "phone2", State: device.StatePaired, Connected: true}, + }) + + set := flag.NewFlagSet("test", flag.ContinueOnError) + if err := set.Parse([]string{"phone2"}); err != nil { + t.Fatal(err) + } + c := cli.NewContext(cli.NewApp(), set, nil) + + got, err := resolveDeviceID(c, cl) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "phone2" { + t.Errorf("resolved %q, want the explicit phone2", got) + } +} diff --git a/cmd/kcd/cli_findmyphone.go b/cmd/kcd/cli_findmyphone.go index b213ee0..377d775 100644 --- a/cmd/kcd/cli_findmyphone.go +++ b/cmd/kcd/cli_findmyphone.go @@ -3,7 +3,6 @@ package main import ( "fmt" - "github.com/bethropolis/kcd/internal/device" "github.com/urfave/cli/v2" ) @@ -17,21 +16,9 @@ var findmyphoneCmd = &cli.Command{ return err } - targetID := c.Args().First() - if targetID == "" { - devs, err := cl.Devices() - if err != nil { - return err - } - for _, d := range devs { - if d.Connected && d.State == device.StatePaired { - targetID = d.ID - break - } - } - if targetID == "" { - return fmt.Errorf("no paired connected devices found") - } + targetID, err := resolveDeviceID(c, cl) + if err != nil { + return err } if err := cl.FindMyPhone(targetID); err != nil { diff --git a/cmd/kcd/cli_lock.go b/cmd/kcd/cli_lock.go index 3fe0d65..683892e 100644 --- a/cmd/kcd/cli_lock.go +++ b/cmd/kcd/cli_lock.go @@ -8,17 +8,18 @@ import ( var lockCmd = &cli.Command{ Name: "lock", - Usage: "Lock the current desktop session", - ArgsUsage: "", + Usage: "Lock the screen of a remote device", + ArgsUsage: "[device-id]", Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - if err := cl.Lock(c.Args().First()); err != nil { + deviceID, err := resolveDeviceID(c, cl) + if err != nil { + return err + } + if err := cl.Lock(deviceID); err != nil { return err } fmt.Println("Lock requested") diff --git a/cmd/kcd/cli_mpris.go b/cmd/kcd/cli_mpris.go index 3c9864c..90fc91f 100644 --- a/cmd/kcd/cli_mpris.go +++ b/cmd/kcd/cli_mpris.go @@ -15,13 +15,24 @@ var actionFlags = []cli.Flag{ &cli.StringFlag{Name: "player", Aliases: []string{"p"}, Usage: "Player name"}, } +// actionDeviceID returns the device for an mpris action subcommand. A +// positional argument wins so `kcd mpris next ` matches every other +// command in the CLI; --device stays as the fallback. (volume and seek +// deliberately do not use this: their positional is the value.) +func actionDeviceID(c *cli.Context) string { + if c.NArg() >= 1 { + return c.Args().First() + } + return c.String("device") +} + func actionCmd(action string) cli.ActionFunc { return func(c *cli.Context) error { cl, err := getClient(c) if err != nil { return err } - return cl.MprisAction(c.String("device"), c.String("player"), action) + return cl.MprisAction(actionDeviceID(c), c.String("player"), action) } } @@ -36,7 +47,7 @@ func skipCmd(action string) cli.ActionFunc { if err != nil { return err } - deviceID := c.String("device") + deviceID := actionDeviceID(c) player := c.String("player") wasPlaying := remotePlayerPlaying(cl, deviceID, player) @@ -193,41 +204,47 @@ var mprisCmd = &cli.Command{ }, }, { - Name: "play", - Usage: "Start playback on a remote device", - Flags: actionFlags, - Action: actionCmd("Play"), + Name: "play", + ArgsUsage: "[device-id]", + Usage: "Start playback on a remote device", + Flags: actionFlags, + Action: actionCmd("Play"), }, { - Name: "pause", - Usage: "Pause playback on a remote device", - Flags: actionFlags, - Action: actionCmd("Pause"), + Name: "pause", + ArgsUsage: "[device-id]", + Usage: "Pause playback on a remote device", + Flags: actionFlags, + Action: actionCmd("Pause"), }, { - Name: "toggle", - Usage: "Toggle play/pause on a remote device", - Flags: actionFlags, - Action: actionCmd("PlayPause"), + Name: "toggle", + ArgsUsage: "[device-id]", + Usage: "Toggle play/pause on a remote device", + Flags: actionFlags, + Action: actionCmd("PlayPause"), }, { - Name: "next", - Usage: "Skip to next track on a remote device (resumes playback if it was playing)", - Flags: actionFlags, - Action: skipCmd("Next"), + Name: "next", + ArgsUsage: "[device-id]", + Usage: "Skip to next track on a remote device (resumes playback if it was playing)", + Flags: actionFlags, + Action: skipCmd("Next"), }, { - Name: "previous", - Aliases: []string{"prev"}, - Usage: "Go to previous track on a remote device (resumes playback if it was playing)", - Flags: actionFlags, - Action: skipCmd("Previous"), + Name: "previous", + ArgsUsage: "[device-id]", + Aliases: []string{"prev"}, + Usage: "Go to previous track on a remote device (resumes playback if it was playing)", + Flags: actionFlags, + Action: skipCmd("Previous"), }, { - Name: "stop", - Usage: "Stop playback on a remote device", - Flags: actionFlags, - Action: actionCmd("Stop"), + Name: "stop", + ArgsUsage: "[device-id]", + Usage: "Stop playback on a remote device", + Flags: actionFlags, + Action: actionCmd("Stop"), }, { Name: "volume", diff --git a/cmd/kcd/cli_ping.go b/cmd/kcd/cli_ping.go index a35a1cc..8f56830 100644 --- a/cmd/kcd/cli_ping.go +++ b/cmd/kcd/cli_ping.go @@ -9,16 +9,17 @@ import ( var pingCmd = &cli.Command{ Name: "ping", Usage: "Send a ping notification to a device", - ArgsUsage: "", + ArgsUsage: "[device-id]", Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - if err := cl.Ping(c.Args().First()); err != nil { + deviceID, err := resolveDeviceID(c, cl) + if err != nil { + return err + } + if err := cl.Ping(deviceID); err != nil { return err } fmt.Println("Ping sent") diff --git a/cmd/kcd/cli_unlock.go b/cmd/kcd/cli_unlock.go index 6583312..9c17ff9 100644 --- a/cmd/kcd/cli_unlock.go +++ b/cmd/kcd/cli_unlock.go @@ -8,17 +8,18 @@ import ( var unlockCmd = &cli.Command{ Name: "unlock", - Usage: "Unlock the current desktop session", - ArgsUsage: "", + Usage: "Unlock the screen of a remote device", + ArgsUsage: "[device-id]", Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - if err := cl.Unlock(c.Args().First()); err != nil { + deviceID, err := resolveDeviceID(c, cl) + if err != nil { + return err + } + if err := cl.Unlock(deviceID); err != nil { return err } fmt.Println("Unlock requested") diff --git a/cmd/kcd/cli_volume.go b/cmd/kcd/cli_volume.go index e076af4..bce4e26 100644 --- a/cmd/kcd/cli_volume.go +++ b/cmd/kcd/cli_volume.go @@ -15,7 +15,7 @@ var volumeCmd = &cli.Command{ { Name: "list", Usage: "List audio sinks on a remote device", - ArgsUsage: "", + ArgsUsage: "[device-id]", Flags: []cli.Flag{ &cli.BoolFlag{ Name: "json", @@ -23,14 +23,15 @@ var volumeCmd = &cli.Command{ }, }, Action: func(c *cli.Context) error { - if c.NArg() < 1 { - return fmt.Errorf("missing device ID") - } cl, err := getClient(c) if err != nil { return err } - data, err := cl.RemoteVolumeList(c.Args().First()) + deviceID, err := resolveDeviceID(c, cl) + if err != nil { + return err + } + data, err := cl.RemoteVolumeList(deviceID) if err != nil { return err } diff --git a/docs/CLI.md b/docs/CLI.md index b6e9595..0f30242 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -202,6 +202,29 @@ kcd devices --json | jq '.[0] | {name, battery: .battery.charge}' --- +## Targeting a device + +Commands that act on a device take it as an optional positional argument: + +```bash +kcd battery [device-id] +kcd ping [device-id] +``` + +Omit the argument and the single paired, connected device is selected, so +the usual one-phone setup needs no ID. With **several** paired devices +connected, `kcd` refuses to guess and lists the candidates: + +```text +multiple devices connected (a1b2c3d4_..., f6e5d4c3_...) — pass a device ID +``` + +Commands with extra positional arguments after the device (`kcd volume set + <0-100>`, `kcd volume mute`) keep the device ID +mandatory, so the remaining arguments stay unambiguous. + +--- + ## connect Manually connect to a device by IP address. Use this when UDP broadcast and mDNS are blocked (corporate Wi-Fi, Docker, university networks). @@ -295,7 +318,7 @@ This sends a rejection packet to the device and removes it from the local device Send a ping notification to a device. The phone displays a "Ping!" notification. ``` -kcd ping +kcd ping [device-id] ``` --- @@ -305,7 +328,7 @@ kcd ping Fetch the current battery level and charging state of a device. ``` -kcd battery [--json] +kcd battery [device-id] [--json] ``` **Example output** @@ -412,7 +435,7 @@ kcd mpris status [--device ] [--json] | Flag | Description | |---|---| -| `--device` | Target device ID (omit for auto-detect) | +| `--device` | Target device ID (omit for auto-detect; the positional `[device-id]` is equivalent and takes precedence) | | `--json` | Output as a JSON array | **Examples** @@ -427,16 +450,16 @@ kcd mpris status --json | jq -r '.[].title' Control playback on the phone. ``` -kcd mpris play [--device ] [--player ] -kcd mpris pause [--device ] [--player ] -kcd mpris toggle [--device ] [--player ] +kcd mpris play [device-id] [--device ] [--player ] +kcd mpris pause [device-id] [--device ] [--player ] +kcd mpris toggle [device-id] [--device ] [--player ] ``` **Flags** | Flag | Description | |---|---| -| `--device` | Target device ID (omit for auto-detect) | +| `--device` | Target device ID (omit for auto-detect; the positional `[device-id]` is equivalent and takes precedence) | | `--player`, `-p` | Player name (omit for auto-fill from cached state) | ### mpris next / prev @@ -444,9 +467,9 @@ kcd mpris toggle [--device ] [--player ] Skip to the next or previous track. If the player was playing, a `Play` action follows the skip to handle phone-side MPRIS implementations that stop after a track change. A paused player is left paused — skipping never starts audio on its own. ``` -kcd mpris next [--device ] [--player ] -kcd mpris previous [--device ] [--player ] -kcd mpris prev [--device ] [--player ] (alias) +kcd mpris next [device-id] [--device ] [--player ] +kcd mpris previous [device-id] [--device ] [--player ] +kcd mpris prev [device-id] [--device ] [--player ] (alias) ``` ### mpris stop @@ -454,7 +477,7 @@ kcd mpris prev [--device ] [--player ] (alias) Stop playback on the phone. ``` -kcd mpris stop [--device ] [--player ] +kcd mpris stop [device-id] [--device ] [--player ] ``` ### mpris volume @@ -635,14 +658,14 @@ kcd findmyphone ## lock / unlock -Lock or unlock the current desktop session. +Ask a remote device to lock or unlock its own screen. ``` -kcd lock -kcd unlock +kcd lock [device-id] +kcd unlock [device-id] ``` -Uses `loginctl lock-session` / `loginctl unlock-session` under the hood. +These send the KDE Connect lock packet to the **remote** device — they do not lock this PC. The reverse direction is separate: when a paired device sends a lock request, the daemon runs `loginctl lock-session` / `loginctl unlock-session` on this desktop. --- @@ -875,7 +898,7 @@ Request a sync round (UID/timestamp list, then vCards for new or changed contacts). Progress arrives as `contacts.updated` events (counts only). ``` -kcd contacts sync +kcd contacts sync [device-id] ``` ### contacts list @@ -884,7 +907,7 @@ List cached contact summaries (empty when never synced — absent means unknown). ``` -kcd contacts list [--json] +kcd contacts list [device-id] [--json] ``` ### contacts clear @@ -893,7 +916,7 @@ Delete a device's cached contacts. Works offline (the cache is local state); re-sync restores everything from the phone. ``` -kcd contacts clear +kcd contacts clear [device-id] ``` --- @@ -907,7 +930,7 @@ Control the remote device's audio volume (requires `remotesystemvolume` plugin). List audio sinks on a remote device and their current volume/mute state. ``` -kcd volume list [--json] +kcd volume list [device-id] [--json] ``` **Example output** From baa21f5e45ad43eec870c0fc8cd958a8a4b78992 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:35:30 +0300 Subject: [PATCH 19/25] fix(completions): add missing fish commands, unpair offline devices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fish completion is the only hand-maintained one — bash and zsh delegate to `kcd --generate-bash-completion` and pick up new commands for free — so it had drifted: dismiss, contacts, and volume were absent from __kcd_cmds and had no completions at all. unpair was also grouped with the commands that complete only connected devices, which is backwards: removing a device that is offline or broken is the main reason to run it. It now completes every known device. Also corrected the lock/unlock descriptions, which claimed to act on the local session, and added positional device completion to the mpris action subcommands. --- packaging/kcd.fish-completion | 78 ++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-) diff --git a/packaging/kcd.fish-completion b/packaging/kcd.fish-completion index d0159f1..d683a18 100644 --- a/packaging/kcd.fish-completion +++ b/packaging/kcd.fish-completion @@ -35,7 +35,8 @@ end # All top-level subcommands (for "don't complete subcommands twice" guards) set -l __kcd_cmds daemon devices connect pair unpair ping battery connectivity watch \ - sftp reply call findmyphone lock unlock share clipboard run sms mpris doctor status + sftp reply dismiss call findmyphone lock unlock share clipboard run sms contacts \ + mpris volume doctor status # --------------------------------------------------------------------------- # Disable file completion globally — we re-enable it only where needed @@ -81,19 +82,25 @@ complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ -a findmyphone -d 'Make the phone play a loud ringtone' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ - -a lock -d 'Lock the current desktop session' + -a lock -d 'Ask a remote device to lock its screen' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ - -a unlock -d 'Unlock the current desktop session' + -a unlock -d 'Ask a remote device to unlock its screen' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ -a share -d 'Send a local file to a device' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ -a clipboard -d 'Sync local clipboard content to a device' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ -a run -d 'Execute and manage remote commands on the device' +complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ + -a dismiss -d 'Dismiss a notification on a device' +complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ + -a contacts -d 'Sync and browse the device address book' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ -a sms -d 'Send an SMS via a device' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ -a mpris -d 'Control media playback on remote devices' +complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ + -a volume -d 'Control remote device volume' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ -a doctor -d 'Check runtime dependencies and configuration' complete -c kcd -n "not __fish_seen_subcommand_from $__kcd_cmds" \ @@ -122,9 +129,14 @@ complete -c kcd -n "__fish_seen_subcommand_from devices" \ complete -c kcd -n "__fish_seen_subcommand_from pair && __kcd_at_arg 2" \ -a '(__kcd_devices)' -d 'Device ID' -# unpair / ping / battery / connectivity / findmyphone / lock / unlock / clipboard +# unpair — completes ALL known devices, not just connected ones: removing +# a device that is offline or broken is the main reason to unpair. +complete -c kcd -n "__fish_seen_subcommand_from unpair && __kcd_at_arg 2" \ + -a '(__kcd_devices)' -d 'Device ID' + +# ping / battery / connectivity / findmyphone / lock / unlock / clipboard # — only make sense with connected devices -for _cmd in unpair ping battery connectivity findmyphone lock unlock clipboard +for _cmd in ping battery connectivity findmyphone lock unlock clipboard complete -c kcd -n "__fish_seen_subcommand_from $_cmd && __kcd_at_arg 2" \ -a '(__kcd_connected_devices)' -d 'Device ID' end @@ -240,6 +252,55 @@ complete -c kcd -n "__fish_seen_subcommand_from run && __fish_seen_subcommand_fr -a '(__kcd_connected_devices)' -d 'Device ID' # command-key at position 4 is free-form +# --------------------------------------------------------------------------- +# contacts sync|list|clear [device-id] +# --------------------------------------------------------------------------- +complete -c kcd -n "__fish_seen_subcommand_from contacts && not __fish_seen_subcommand_from sync list clear" \ + -a sync -d 'Request a contacts sync from a device' +complete -c kcd -n "__fish_seen_subcommand_from contacts && not __fish_seen_subcommand_from sync list clear" \ + -a list -d 'List cached contacts for a device' +complete -c kcd -n "__fish_seen_subcommand_from contacts && not __fish_seen_subcommand_from sync list clear" \ + -a clear -d 'Delete cached contacts for a device' + +# list reads the local cache, so it completes all known devices; sync and +# clear need the device online. +complete -c kcd -n "__fish_seen_subcommand_from contacts && __fish_seen_subcommand_from list && __kcd_at_arg 3" \ + -a '(__kcd_devices)' -d 'Device ID' +for _sub in sync clear + complete -c kcd -n "__fish_seen_subcommand_from contacts && __fish_seen_subcommand_from $_sub && __kcd_at_arg 3" \ + -a '(__kcd_connected_devices)' -d 'Device ID' +end +complete -c kcd -n "__fish_seen_subcommand_from contacts && __fish_seen_subcommand_from list" \ + -l json -d 'Output raw JSON' + +# --------------------------------------------------------------------------- +# dismiss [device-id] +# --------------------------------------------------------------------------- +complete -c kcd -n "__fish_seen_subcommand_from dismiss && __kcd_at_arg 2" \ + -a '(__kcd_connected_devices)' -d 'Device ID' +# notification-id at position 3 is free-form + +# --------------------------------------------------------------------------- +# volume list|set|mute [device-id] ... +# --------------------------------------------------------------------------- +complete -c kcd -n "__fish_seen_subcommand_from volume && not __fish_seen_subcommand_from list set mute" \ + -a list -d 'List audio sinks on a remote device' +complete -c kcd -n "__fish_seen_subcommand_from volume && not __fish_seen_subcommand_from list set mute" \ + -a set -d 'Set volume on a remote device sink' +complete -c kcd -n "__fish_seen_subcommand_from volume && not __fish_seen_subcommand_from list set mute" \ + -a mute -d 'Mute or unmute a remote device sink' + +# list auto-resolves the device; set/mute keep it mandatory so their +# remaining positional arguments stay unambiguous. +complete -c kcd -n "__fish_seen_subcommand_from volume && __fish_seen_subcommand_from list && __kcd_at_arg 3" \ + -a '(__kcd_connected_devices)' -d 'Device ID' +for _sub in set mute + complete -c kcd -n "__fish_seen_subcommand_from volume && __fish_seen_subcommand_from $_sub && __kcd_at_arg 3" \ + -a '(__kcd_connected_devices)' -d 'Device ID' +end +complete -c kcd -n "__fish_seen_subcommand_from volume && __fish_seen_subcommand_from list" \ + -l json -d 'Output raw JSON' + # --------------------------------------------------------------------------- # sms send|conversations|conversation|attachment [...] # --------------------------------------------------------------------------- @@ -305,6 +366,13 @@ for _sub in play pause toggle next previous prev stop volume seek status -l player -s p -d 'Player name' -r end +# The action subcommands also take the device as an optional positional +# (kcd mpris next ); volume and seek keep theirs for the value. +for _sub in play pause toggle next previous prev stop + complete -c kcd -n "__fish_seen_subcommand_from mpris && __fish_seen_subcommand_from $_sub && __kcd_at_arg 3" \ + -a '(__kcd_connected_devices)' -d 'Device ID' +end + # --------------------------------------------------------------------------- # status # --------------------------------------------------------------------------- From 2e2dd81e279d3342d4fe62de5d42a776b43c0791 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:56:53 +0300 Subject: [PATCH 20/25] fix(install): derive the version from the newest release tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git describe --tags --always --dirty` reports the newest tag reachable from HEAD, not the newest release. dev/next forked at fb81751, before the v1.19.1 release merge on main, so describe walked past v1.19.1 and labelled the build v1.18.2-42-gbaa21f5 — 1.19.x code reported as 1.18.2. The base is now the newest v* tag in the repo: v1.20.0 checkout exactly on a release tag v1.19.1+42.gbaa21f5 N commits past that tag on this branch v1.19.1-dev+gbaa21f5 branch that forked before the newest tag dev no tags, or not a git checkout The forked case deliberately reports no commit count: the tag is not an ancestor, so any number would be fiction. -dirty is still appended for an uncommitted tree; bin/ is gitignored so the build output itself does not trigger it. --- scripts/install.sh | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/scripts/install.sh b/scripts/install.sh index 659abdb..7eb0ab7 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -101,7 +101,48 @@ step "Building static binary" cd "${REPO_ROOT}" -VERSION="$(git describe --tags --always --dirty 2>/dev/null || echo 'dev')" +# Version follows the git tag, never a hand-maintained constant: +# v1.20.0 checkout sitting exactly on a release tag +# v1.19.1+42.gbaa21f5 N commits after that tag on this branch +# v1.19.1-dev+gbaa21f5 branch that forked before the newest tag +# dev no tags reachable at all +# +# The base is the newest release tag in the repo, not the newest one +# reachable from HEAD. dev/next forks before the release merge, so +# `git describe` alone walks past v1.19.1 and reports the ancient v1.18.2 +# the branch also descends from — labelling v1.19.x+ code as 1.18.2. +resolve_version() { + local short_sha exact latest_tag ahead + short_sha="$(git rev-parse --short HEAD 2>/dev/null || echo unknown)" + + exact="$(git describe --tags --exact-match HEAD 2>/dev/null || true)" + if [ -n "$exact" ]; then + VERSION="$exact" + return + fi + + latest_tag="$(git tag --list 'v[0-9]*' --sort=-v:refname 2>/dev/null | head -n 1)" + if [ -z "$latest_tag" ]; then + VERSION="dev" + elif git merge-base --is-ancestor "$latest_tag" HEAD 2>/dev/null; then + ahead="$(git rev-list --count "${latest_tag}..HEAD" 2>/dev/null || echo 0)" + VERSION="${latest_tag}+${ahead}.g${short_sha}" + else + # Not in this branch's history, so a commit count would be fiction. + VERSION="${latest_tag}-dev+g${short_sha}" + fi +} + +VERSION="dev" +if command -v git >/dev/null 2>&1 && git rev-parse --git-dir >/dev/null 2>&1; then + resolve_version + if [ -n "$(git status --porcelain 2>/dev/null)" ]; then + VERSION="${VERSION}-dirty" + fi +else + warn "Not a git checkout — building as 'dev' (no tag information available)." +fi + COMMIT="$(git rev-parse --short HEAD 2>/dev/null || echo 'unknown')" DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" From 66c352cd39de109740a31fa7d15fff1da5d5a86d Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 03:59:10 +0300 Subject: [PATCH 21/25] fix(mpris): break the poller deadlock on a lost PlaybackStatus signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live testing found the position poller never armed when playback started. Root cause is a self-sustaining deadlock: syncPlayingPollerLocked gated the arm on the cached IsPlaying flag, but that cache is refreshed only by a signal or by the poller itself. Lose one PlaybackStatus signal and the cache stays "paused", so the poller stays disarmed, so nothing refreshes the cache — and no further PlaybackStatus signal arrives until the next pause/play. The removed 2s timer used to break this by refreshing unconditionally; deleting it removed the safety net. - Arm on any observed change rather than on the cached flag. The poller verifies against live D-Bus reads, so a stale cache costs one tick. - Let the poller stop itself once a live read shows nothing playing, so its lifetime follows the player instead of the cache that armed it. - Guard the self-stop with a generation token; otherwise a poller that stops on its own clears its successor's handle and a second ticker runs unstoppable. - Narrow the NameOwnerChanged match to arg0prefix org.mpris.MediaPlayer2. It previously matched every name acquired on the session bus, each handled with blocking D-Bus calls, and godbus drops signals when its 64-slot buffer overflows. A dropped signal is how the deadlock started. - Log the initial player-listing error, previously discarded with `_`. Measured after the fix: local playback arms the poller (~32 GetAll/min at the 2s interval) and idle stays at 0.00 CPU ticks/min with zero context switches. --- internal/plugins/mpris/discovery.go | 13 +- internal/plugins/mpris/local.go | 5 +- internal/plugins/mpris/mpris.go | 5 +- internal/plugins/mpris/playing_poller.go | 142 ++++++++++++------ internal/plugins/mpris/playing_poller_test.go | 141 ++++++++++++----- internal/plugins/mpris/signals.go | 4 +- internal/plugins/mpris/watcher.go | 20 ++- 7 files changed, 230 insertions(+), 100 deletions(-) diff --git a/internal/plugins/mpris/discovery.go b/internal/plugins/mpris/discovery.go index ef43124..1626c8a 100644 --- a/internal/plugins/mpris/discovery.go +++ b/internal/plugins/mpris/discovery.go @@ -13,6 +13,11 @@ type playerEntry struct { identity string } +// mprisBusPrefix is the well-known-name prefix every MPRIS player +// advertises. Matching on it keeps the watcher's NameOwnerChanged +// subscription off unrelated session-bus traffic. +const mprisBusPrefix = "org.mpris.MediaPlayer2." + func listPlayersDBus(conn *dbus.Conn) ([]playerEntry, error) { if conn == nil { return nil, nil @@ -25,17 +30,17 @@ func listPlayersDBus(conn *dbus.Conn) ([]playerEntry, error) { var all []playerEntry for _, name := range names { - if !strings.HasPrefix(name, "org.mpris.MediaPlayer2.") { + if !strings.HasPrefix(name, mprisBusPrefix) { continue } - if strings.HasPrefix(name, "org.mpris.MediaPlayer2.kdeconnect.") { + if strings.HasPrefix(name, mprisBusPrefix+"kdeconnect.") { continue } if name == "org.mpris.MediaPlayer2.playerctld" { continue } - short := strings.TrimPrefix(name, "org.mpris.MediaPlayer2.") + short := strings.TrimPrefix(name, mprisBusPrefix) if idx := strings.Index(short, ".instance"); idx != -1 { short = short[:idx] } @@ -89,7 +94,7 @@ func listPlayersDBus(conn *dbus.Conn) ([]playerEntry, error) { } func resolveIdentity(conn *dbus.Conn, busName string) *playerEntry { - short := strings.TrimPrefix(busName, "org.mpris.MediaPlayer2.") + short := strings.TrimPrefix(busName, mprisBusPrefix) if idx := strings.Index(short, ".instance"); idx != -1 { short = short[:idx] } diff --git a/internal/plugins/mpris/local.go b/internal/plugins/mpris/local.go index 90727c6..68c08d6 100644 --- a/internal/plugins/mpris/local.go +++ b/internal/plugins/mpris/local.go @@ -123,8 +123,9 @@ func (p *MPRISPlugin) removePlayer(displayName string) { delete(p.players, displayName) delete(p.lastTracks, displayName) delete(p.lastStates, displayName) - // A removal may take the last playing player with it — disarm the - // poller so a removed player can't pin the ticker on. + // With no players left there is nothing to poll, so stop now rather + // than let the poller discover it on its next tick. sync handles + // the case where other players remain. p.syncPlayingPollerLocked() p.mu.Unlock() diff --git a/internal/plugins/mpris/mpris.go b/internal/plugins/mpris/mpris.go index bb4ad09..5ad7a61 100644 --- a/internal/plugins/mpris/mpris.go +++ b/internal/plugins/mpris/mpris.go @@ -35,9 +35,12 @@ type MPRISPlugin struct { // mprisCfg gates the position poller: with PollWhilePlaying the // ticker exists only while at least one local player IsPlaying. - // pollCancel stops it; nil means no poller is running. + // pollCancel stops it; nil means no poller is running. pollGen + // identifies the current poller so a self-stopping one does not + // clear its successor's handle. mprisCfg config.MPRISConfig pollCancel context.CancelFunc + pollGen uint64 // remotePollCancel stops the remote-state poller; nil means it is // not running. The poller is demand-driven (see syncRemotePoller): diff --git a/internal/plugins/mpris/playing_poller.go b/internal/plugins/mpris/playing_poller.go index 82dc4c7..f4debba 100644 --- a/internal/plugins/mpris/playing_poller.go +++ b/internal/plugins/mpris/playing_poller.go @@ -9,9 +9,9 @@ import ( ) // storeLocalState caches the latest known state for a local player, then -// arms or disarms the position poller. It is the single choke point for -// all lastStates writes so the poller can never drift out of sync with -// what is actually playing. +// syncs the position poller. It is the single choke point for all +// lastStates writes, so every observed change — signal or poll — has a +// chance to (re)arm the poller. func (p *MPRISPlugin) storeLocalState(displayName string, state *NowPlaying) { p.mu.Lock() p.lastStates[displayName] = state @@ -19,34 +19,48 @@ func (p *MPRISPlugin) storeLocalState(displayName string, state *NowPlaying) { p.mu.Unlock() } -// syncPlayingPollerLocked starts the position ticker when at least one -// tracked player IsPlaying and stops it otherwise. With -// PollWhilePlaying=false the poller never runs (pure event-driven). +// syncPlayingPollerLocked reconciles the poller with what we know. +// +// It deliberately does NOT trust the cached IsPlaying flag to decide +// whether to run. The cache is only refreshed by a signal or by the +// poller itself, so gating on it deadlocks: lose one PlaybackStatus +// signal and the cache stays "paused", so the poller never arms, so +// nothing ever refreshes the cache — and no further PlaybackStatus +// signal arrives until the next pause/play. The removed 2s timer used +// to break that deadlock by refreshing unconditionally. +// +// Instead any observed change arms the poller, and the poller decides +// its own lifetime from live D-Bus reads. A paused desktop pays one +// extra tick per signal, then goes quiet. +// // Callers must hold p.mu. func (p *MPRISPlugin) syncPlayingPollerLocked() { if !p.mprisCfg.PollWhilePlaying { p.stopPlayingPollerLocked() return } - playing := false - for _, s := range p.lastStates { - if s != nil && s.IsPlaying { - playing = true - break - } - } - switch { - case playing && p.pollCancel == nil: - ctx, cancel := context.WithCancel(p.watchCtx) - p.pollCancel = cancel - interval := config.Duration(p.mprisCfg.PositionInterval) - p.logger.Debug("mpris: arming position poller", - log.Duration("interval", interval)) - go p.runPlayingPoller(ctx, interval) - case !playing && p.pollCancel != nil: - p.logger.Debug("mpris: disarming position poller") + if len(p.players) == 0 { p.stopPlayingPollerLocked() + return } + p.armPlayingPollerLocked() +} + +// armPlayingPollerLocked starts the position ticker if it is not already +// running. Callers must hold p.mu. +func (p *MPRISPlugin) armPlayingPollerLocked() { + if p.pollCancel != nil { + return + } + ctx, cancel := context.WithCancel(p.watchCtx) + p.pollCancel = cancel + // A generation token lets a poller that stops itself avoid clearing + // the handle of the poller that replaced it. + p.pollGen++ + gen := p.pollGen + interval := config.Duration(p.mprisCfg.PositionInterval) + p.logger.Debug("mpris: arming position poller", log.Duration("interval", interval)) + go p.runPlayingPoller(ctx, interval, gen) } // stopPlayingPollerLocked cancels a running position poller, if any. @@ -59,43 +73,71 @@ func (p *MPRISPlugin) stopPlayingPollerLocked() { } } -// runPlayingPoller re-reads D-Bus state for playing players only, at -// PositionInterval, until ctx is cancelled (pause/stop/removal) or the -// plugin shuts down. Paused players and an empty player list cost zero -// wakeups — that is the entire point: idle desktops stay silent. -func (p *MPRISPlugin) runPlayingPoller(ctx context.Context, interval time.Duration) { +// runPlayingPoller re-reads D-Bus state for every tracked player at +// PositionInterval. It returns once a live read says nothing is playing, +// so its lifetime follows the player rather than the cache that armed +// it. Paused players and an empty player list cost zero wakeups — that is +// the entire point: idle desktops stay silent. +func (p *MPRISPlugin) runPlayingPoller(ctx context.Context, interval time.Duration, gen uint64) { ticker := time.NewTicker(interval) defer ticker.Stop() + defer p.finishPlayingPoller(gen) + for { select { case <-ctx.Done(): return case <-ticker.C: - p.mu.RLock() - playing := make([]*trackedPlayer, 0, len(p.players)) - for _, pl := range p.players { - if s := p.lastStates[pl.displayName]; s != nil && s.IsPlaying { - playing = append(playing, pl) - } + if !p.pollPlayingPlayers() { + p.logger.Debug("mpris: no player is playing, stopping position poller") + return } - p.mu.RUnlock() + } + } +} - for _, pl := range playing { - state, err := p.playerState(pl.displayName) - if err != nil { - continue - } - // Hold RLock through the compare: it reads the cached - // anchor, which broadcast stamps under p.mu. - p.mu.RLock() - changed := localStateChanged(time.Now().UnixMilli(), state, p.lastStates[pl.displayName]) - p.mu.RUnlock() +// finishPlayingPoller releases the poller slot when this poller stops on +// its own. It only clears the handle if a later arm has not already +// replaced it, so a self-stopping poller cannot orphan its successor. +func (p *MPRISPlugin) finishPlayingPoller(gen uint64) { + p.mu.Lock() + defer p.mu.Unlock() + if p.pollGen == gen { + p.pollCancel = nil + } +} - if changed { - p.storeLocalState(pl.displayName, state) - p.broadcast(state) - } - } +// pollPlayingPlayers samples every tracked player once, broadcasting the +// ones whose state actually changed. It reports whether anything is +// playing according to those live reads — the sole input to the +// poller's stop decision. +func (p *MPRISPlugin) pollPlayingPlayers() bool { + p.mu.RLock() + players := make([]*trackedPlayer, 0, len(p.players)) + for _, pl := range p.players { + players = append(players, pl) + } + p.mu.RUnlock() + + playing := false + for _, pl := range players { + state, err := p.playerState(pl.displayName) + if err != nil { + continue + } + if state.IsPlaying { + playing = true + } + // Hold RLock through the compare: it reads the cached anchor, + // which broadcast stamps under p.mu. + p.mu.RLock() + changed := localStateChanged(time.Now().UnixMilli(), state, p.lastStates[pl.displayName]) + p.mu.RUnlock() + + if changed { + p.storeLocalState(pl.displayName, state) + p.broadcast(state) } } + return playing } diff --git a/internal/plugins/mpris/playing_poller_test.go b/internal/plugins/mpris/playing_poller_test.go index c512383..0dad49c 100644 --- a/internal/plugins/mpris/playing_poller_test.go +++ b/internal/plugins/mpris/playing_poller_test.go @@ -13,65 +13,128 @@ func testMPRISConfig() config.MPRISConfig { return config.MPRISConfig{PollWhilePlaying: true, PositionInterval: "1h"} } -// The poller must exist exactly while at least one tracked player reports -// IsPlaying — and never otherwise. -func TestPollArmsOnlyWhilePlaying(t *testing.T) { +func pollerArmed(p *MPRISPlugin) bool { + p.mu.RLock() + defer p.mu.RUnlock() + return p.pollCancel != nil +} + +func trackPlayer(p *MPRISPlugin, name string) { + p.mu.Lock() + p.players[name] = &trackedPlayer{displayName: name} + p.mu.Unlock() +} + +// An observed state change must arm the poller even when the cached +// IsPlaying flag says paused. This is the regression guard: gating the +// arm on the cache deadlocks, because a single lost PlaybackStatus +// signal leaves the cache stale forever and the poller — the only other +// thing that refreshes it — disarmed. +func TestPollArmsOnObservedChange(t *testing.T) { p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, testMPRISConfig(), log.Nop()) + defer p.watchCancel() - p.mu.RLock() - armed := p.pollCancel != nil - p.mu.RUnlock() - if armed { - t.Fatal("poller armed with zero players") + trackPlayer(p, "Nightdrive") + if pollerArmed(p) { + t.Fatal("poller armed before any state was observed") } - p.storeLocalState("Paused FM", &NowPlaying{Player: "Paused FM", IsPlaying: false}) + // Cached state says paused, but something changed, so the poller must + // start and verify against live D-Bus state itself. + p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: false}) + if !pollerArmed(p) { + t.Fatal("poller not armed after an observed change (deadlock regression)") + } +} - p.mu.RLock() - armed = p.pollCancel != nil - p.mu.RUnlock() - if armed { - t.Fatal("poller armed while only paused players tracked") +// With no tracked players there is nothing to poll, so no poller. +func TestPollNotArmedWithoutPlayers(t *testing.T) { + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, testMPRISConfig(), log.Nop()) + defer p.watchCancel() + + p.storeLocalState("Ghost FM", &NowPlaying{Player: "Ghost FM", IsPlaying: true}) + if pollerArmed(p) { + t.Fatal("poller armed with zero tracked players") } +} - p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: true}) +// The poller must stop itself once a live read shows nothing playing. +// Here D-Bus is unavailable, so every live read fails — the same path a +// paused player takes, since a read that reports IsPlaying=false is what +// keeps the ticker alive. +func TestPollStopsItselfWhenNothingPlaying(t *testing.T) { + cfg := testMPRISConfig() + cfg.PositionInterval = "10ms" + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, cfg, log.Nop()) + defer p.watchCancel() + + trackPlayer(p, "Paused FM") + p.storeLocalState("Paused FM", &NowPlaying{Player: "Paused FM", IsPlaying: false}) + if !pollerArmed(p) { + t.Fatal("poller not armed after an observed change") + } - p.mu.RLock() - armed = p.pollCancel != nil - p.mu.RUnlock() - if !armed { - t.Fatal("poller not armed while a player IsPlaying") + deadline := time.Now().Add(3 * time.Second) + for pollerArmed(p) && time.Now().Before(deadline) { + time.Sleep(5 * time.Millisecond) + } + if pollerArmed(p) { + t.Fatal("poller still armed after live reads reported nothing playing") } +} - // Pausing the last playing player must disarm (the 1h ticker never - // fires, so no D-Bus traffic can occur during this test). - p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: false}) +// A self-stopping poller must not clear the handle of the poller that +// replaced it, or the successor becomes unstoppable and a second ticker +// keeps running. +func TestSelfStoppingPollerDoesNotOrphanSuccessor(t *testing.T) { + cfg := testMPRISConfig() + cfg.PositionInterval = "10ms" + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, cfg, log.Nop()) + defer p.watchCancel() + + trackPlayer(p, "Paused FM") + p.storeLocalState("Paused FM", &NowPlaying{Player: "Paused FM", IsPlaying: false}) + + // Let the first poller arm, then simulate it self-stopping after a + // successor has already taken the slot. + p.mu.Lock() + firstGen := p.pollGen + p.pollGen++ + successorGen := p.pollGen + p.mu.Unlock() + + p.finishPlayingPoller(firstGen) p.mu.RLock() - armed = p.pollCancel != nil + cleared := p.pollCancel == nil p.mu.RUnlock() - if armed { - t.Fatal("poller still armed after last player paused") + if cleared { + t.Fatal("a stale poller cleared its successor's handle") + } + + // The current generation may clear the slot. + p.finishPlayingPoller(successorGen) + if pollerArmed(p) { + t.Fatal("current poller generation failed to release the slot") } } -// Removing the last playing player must disarm even though no state +// Removing the last player must stop the poller even though no state // update flows through storeLocalState. func TestPollDisarmsOnPlayerRemoval(t *testing.T) { p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, testMPRISConfig(), log.Nop()) + defer p.watchCancel() - p.mu.Lock() - p.players["Nightdrive"] = &trackedPlayer{displayName: "Nightdrive"} - p.mu.Unlock() + trackPlayer(p, "Nightdrive") p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: true}) + if !pollerArmed(p) { + t.Fatal("poller not armed before removal") + } p.removePlayer("Nightdrive") - p.mu.RLock() - armed := p.pollCancel != nil - p.mu.RUnlock() - if armed { - t.Fatal("poller still armed after playing player removed") + if pollerArmed(p) { + t.Fatal("poller still armed after last player removed") } } @@ -79,13 +142,12 @@ func TestPollDisarmsOnPlayerRemoval(t *testing.T) { // event-driven mode). func TestPollNeverArmsWhenDisabled(t *testing.T) { p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) + defer p.watchCancel() + trackPlayer(p, "Nightdrive") p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: true}) - p.mu.RLock() - armed := p.pollCancel != nil - p.mu.RUnlock() - if armed { + if pollerArmed(p) { t.Fatal("poller armed with PollWhilePlaying=false") } } @@ -160,6 +222,7 @@ func TestLocalStateChangedPausedPosition(t *testing.T) { // Anchor is stamped on the broadcast state itself. func TestBroadcastAnchorFreshness(t *testing.T) { p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, config.MPRISConfig{}, log.Nop()) + defer p.watchCancel() state := &NowPlaying{Player: "Nightdrive", Pos: 42000, IsPlaying: true} before := time.Now().UnixMilli() diff --git a/internal/plugins/mpris/signals.go b/internal/plugins/mpris/signals.go index 17e230c..bbb5f1b 100644 --- a/internal/plugins/mpris/signals.go +++ b/internal/plugins/mpris/signals.go @@ -17,8 +17,8 @@ func (p *MPRISPlugin) handleNameOwnerChanged(sig *dbus.Signal, conn *dbus.Conn, oldOwner, _ := sig.Body[1].(string) newOwner, _ := sig.Body[2].(string) - if !strings.HasPrefix(name, "org.mpris.MediaPlayer2.") || - strings.HasPrefix(name, "org.mpris.MediaPlayer2.kdeconnect.") || + if !strings.HasPrefix(name, mprisBusPrefix) || + strings.HasPrefix(name, mprisBusPrefix+"kdeconnect.") || name == "org.mpris.MediaPlayer2.playerctld" { return } diff --git a/internal/plugins/mpris/watcher.go b/internal/plugins/mpris/watcher.go index 91e2949..858654c 100644 --- a/internal/plugins/mpris/watcher.go +++ b/internal/plugins/mpris/watcher.go @@ -40,7 +40,14 @@ func (p *MPRISPlugin) runDBusWatcher(ctx context.Context) error { uniqueToDisplay := make(map[string]string) - entries, _ := listPlayersDBus(p.dbus) + entries, err := listPlayersDBus(p.dbus) + if err != nil { + // Not fatal: the watcher keeps running so NameOwnerChanged can + // still pick players up as they appear. Logging matters — this + // error used to be discarded, leaving an empty tracker with no + // clue why. + p.logger.Warn("mpris: initial player listing failed", log.Error(err)) + } for _, e := range entries { var owner string if err := conn.BusObject().Call("org.freedesktop.DBus.GetNameOwner", 0, e.busName).Store(&owner); err == nil { @@ -63,14 +70,23 @@ func (p *MPRISPlugin) runDBusWatcher(ctx context.Context) error { } } + // Register the name watch BEFORE anything can block, and narrow it to + // MPRIS names. An unfiltered NameOwnerChanged match delivers a signal + // for every name acquired on the session bus, and each one is handled + // synchronously with blocking D-Bus calls — enough volume overflows + // the signal buffer, and godbus drops the overflow. A dropped + // PlaybackStatus signal used to strand the position poller disarmed. if err := conn.AddMatchSignal( dbus.WithMatchInterface("org.freedesktop.DBus"), dbus.WithMatchMember("NameOwnerChanged"), + dbus.WithMatchOption("arg0prefix", mprisBusPrefix), ); err != nil { return err } - ch := make(chan *dbus.Signal, 64) + // Sized for a burst of player churn (browser restarts, several + // players at once) rather than bus-wide name traffic. + ch := make(chan *dbus.Signal, 256) conn.Signal(ch) for { From 108ba2607fc9fd8991e77b61211b2aaf9a955772 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 04:21:21 +0300 Subject: [PATCH 22/25] fix(mpris): don't stop the poller on a failed D-Bus read; drop bad match rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live testing of 66c352c found two defects, both caught on the running daemon rather than in tests. The arg0prefix match rule in 66c352c is not valid D-Bus syntax — match rules have no string-prefix key — so AddMatchSignal failed and the watcher crash-looped every 3s with "Invalid match rule". Reverted to the unfiltered name watch; non-MPRIS signals are already rejected in the handler on a prefix check before any blocking work, so the flood is not worth a filter the bus cannot express. The larger buffer stays. More seriously, the poller treated a failed live read as "nothing is playing" and exited. Firefox's MPRIS endpoint answers intermittently, so a single hiccup stopped sampling and, with no further signal while playback continues, the poller never restarted — the same strand the previous commit set out to remove, just reached by a different road. pollPlayingPlayers now reports completeness separately: only a full set of successful reads reporting no playback ends the ticker. Failed reads retry, bounded by maxConsecutiveReadFailures so a name outliving a dead object cannot pin a ticker on. Verified live after restart, with no reconcile and no manual nudge: Firefox discovered by the initial listing, poller armed on its own at 17 GetAll/30s (~34/min, the 2s interval), 2.22 CPU ticks/min while playing, 0 voluntary context switches. --- internal/plugins/mpris/playing_poller.go | 52 +++++++++++++++---- internal/plugins/mpris/playing_poller_test.go | 28 ++++++---- internal/plugins/mpris/watcher.go | 13 ++--- 3 files changed, 65 insertions(+), 28 deletions(-) diff --git a/internal/plugins/mpris/playing_poller.go b/internal/plugins/mpris/playing_poller.go index f4debba..f5496c0 100644 --- a/internal/plugins/mpris/playing_poller.go +++ b/internal/plugins/mpris/playing_poller.go @@ -73,24 +73,47 @@ func (p *MPRISPlugin) stopPlayingPollerLocked() { } } +// maxConsecutiveReadFailures bounds how long the poller keeps retrying +// when every live read errors. A player that is merely slow to answer +// must not end sampling; one that is gone for good should not pin a +// ticker on. Its name disappearing handles that case, so this is only a +// backstop for a name that lingers with a dead object behind it. +const maxConsecutiveReadFailures = 5 + // runPlayingPoller re-reads D-Bus state for every tracked player at -// PositionInterval. It returns once a live read says nothing is playing, -// so its lifetime follows the player rather than the cache that armed -// it. Paused players and an empty player list cost zero wakeups — that is -// the entire point: idle desktops stay silent. +// PositionInterval. It returns once live reads say nothing is playing, so +// its lifetime follows the player rather than the cache that armed it. +// Paused players and an empty player list cost zero wakeups — that is the +// entire point: idle desktops stay silent. func (p *MPRISPlugin) runPlayingPoller(ctx context.Context, interval time.Duration, gen uint64) { ticker := time.NewTicker(interval) defer ticker.Stop() defer p.finishPlayingPoller(gen) + failures := 0 for { select { case <-ctx.Done(): return case <-ticker.C: - if !p.pollPlayingPlayers() { + playing, complete := p.pollPlayingPlayers() + switch { + case playing: + failures = 0 + case complete: + // Every player answered and none is playing: a real stop. p.logger.Debug("mpris: no player is playing, stopping position poller") return + default: + // A read failed. That is not evidence playback ended — + // Firefox's MPRIS endpoint answers intermittently — so keep + // sampling, but do not do it forever. + failures++ + if failures >= maxConsecutiveReadFailures { + p.logger.Warn("mpris: position poller gave up after repeated read failures", + log.Int("failures", failures)) + return + } } } } @@ -108,10 +131,12 @@ func (p *MPRISPlugin) finishPlayingPoller(gen uint64) { } // pollPlayingPlayers samples every tracked player once, broadcasting the -// ones whose state actually changed. It reports whether anything is -// playing according to those live reads — the sole input to the -// poller's stop decision. -func (p *MPRISPlugin) pollPlayingPlayers() bool { +// ones whose state actually changed. +// +// It reports whether anything is playing, and whether every tracked +// player answered. An unanswered player makes the result incomplete: the +// caller must not read that as "playback ended". +func (p *MPRISPlugin) pollPlayingPlayers() (playing, complete bool) { p.mu.RLock() players := make([]*trackedPlayer, 0, len(p.players)) for _, pl := range p.players { @@ -119,10 +144,15 @@ func (p *MPRISPlugin) pollPlayingPlayers() bool { } p.mu.RUnlock() - playing := false + if len(players) == 0 { + return false, true + } + + complete = true for _, pl := range players { state, err := p.playerState(pl.displayName) if err != nil { + complete = false continue } if state.IsPlaying { @@ -139,5 +169,5 @@ func (p *MPRISPlugin) pollPlayingPlayers() bool { p.broadcast(state) } } - return playing + return playing, complete } diff --git a/internal/plugins/mpris/playing_poller_test.go b/internal/plugins/mpris/playing_poller_test.go index 0dad49c..4e37b94 100644 --- a/internal/plugins/mpris/playing_poller_test.go +++ b/internal/plugins/mpris/playing_poller_test.go @@ -58,28 +58,38 @@ func TestPollNotArmedWithoutPlayers(t *testing.T) { } } -// The poller must stop itself once a live read shows nothing playing. -// Here D-Bus is unavailable, so every live read fails — the same path a -// paused player takes, since a read that reports IsPlaying=false is what -// keeps the ticker alive. -func TestPollStopsItselfWhenNothingPlaying(t *testing.T) { +// A live read that FAILS is not evidence that playback ended — Firefox's +// MPRIS endpoint answers intermittently, and treating an error as "not +// playing" used to strand the poller on the first hiccup. The poller must +// retry through failures and only give up after the bounded backstop. +func TestPollSurvivesReadFailures(t *testing.T) { cfg := testMPRISConfig() cfg.PositionInterval = "10ms" p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, cfg, log.Nop()) defer p.watchCancel() - trackPlayer(p, "Paused FM") - p.storeLocalState("Paused FM", &NowPlaying{Player: "Paused FM", IsPlaying: false}) + // With no usable D-Bus connection every read fails, so this exercises + // the failure path exactly. + trackPlayer(p, "Flaky FM") + p.storeLocalState("Flaky FM", &NowPlaying{Player: "Flaky FM", IsPlaying: true}) if !pollerArmed(p) { t.Fatal("poller not armed after an observed change") } - deadline := time.Now().Add(3 * time.Second) + // It must still be running well past the first failure. + time.Sleep(maxConsecutiveReadFailures * 3 * time.Millisecond) + if !pollerArmed(p) { + t.Fatal("poller stopped on a read failure instead of retrying") + } + + // The bounded backstop must still release the slot rather than run + // forever on a name with a dead object behind it. + deadline := time.Now().Add(5 * time.Second) for pollerArmed(p) && time.Now().Before(deadline) { time.Sleep(5 * time.Millisecond) } if pollerArmed(p) { - t.Fatal("poller still armed after live reads reported nothing playing") + t.Fatal("poller never gave up after the bounded failure backstop") } } diff --git a/internal/plugins/mpris/watcher.go b/internal/plugins/mpris/watcher.go index 858654c..23066b0 100644 --- a/internal/plugins/mpris/watcher.go +++ b/internal/plugins/mpris/watcher.go @@ -70,22 +70,19 @@ func (p *MPRISPlugin) runDBusWatcher(ctx context.Context) error { } } - // Register the name watch BEFORE anything can block, and narrow it to - // MPRIS names. An unfiltered NameOwnerChanged match delivers a signal - // for every name acquired on the session bus, and each one is handled - // synchronously with blocking D-Bus calls — enough volume overflows - // the signal buffer, and godbus drops the overflow. A dropped - // PlaybackStatus signal used to strand the position poller disarmed. + // Watch every name change and filter to MPRIS in the handler. D-Bus + // match rules have no string-prefix key, and the non-MPRIS signals + // this lets through are cheap — the handler rejects them on a prefix + // check before any blocking work. if err := conn.AddMatchSignal( dbus.WithMatchInterface("org.freedesktop.DBus"), dbus.WithMatchMember("NameOwnerChanged"), - dbus.WithMatchOption("arg0prefix", mprisBusPrefix), ); err != nil { return err } // Sized for a burst of player churn (browser restarts, several - // players at once) rather than bus-wide name traffic. + // players appearing at once). ch := make(chan *dbus.Signal, 256) conn.Signal(ch) From 3adabf28f10d0281ec79241d82a5e3c3ca0b2012 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 04:55:52 +0300 Subject: [PATCH 23/25] ci: run integration tests on pull requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integration job was gated on `github.event_name == 'push'`, but the push trigger is scoped to main/master — so a feature branch only ever fires pull_request, and integration never ran before merge. It would first have executed on the post-merge main push, with the release branch already carrying the change. --- .github/workflows/ci.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b78cd9e..ca668f4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -116,7 +116,10 @@ jobs: integration-test: name: Integration Tests runs-on: ubuntu-latest - if: github.event_name == 'push' + # Runs on pull_request too. `push` is scoped to main/master, so a + # feature branch only ever fires the pull_request event — gating on + # push alone meant integration never ran before merge. + if: github.event_name == 'push' || github.event_name == 'pull_request' needs: test steps: - uses: actions/checkout@v4 From f2023dda7bf5c2bf3f466577a1b95d97d96691b1 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 05:29:07 +0300 Subject: [PATCH 24/25] fix(mpris): re-arm a stranded position poller with a slow watchdog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The poller arms on an observed change, stops on a confirmed pause, and only restarts when another change re-arms it. That leaves it hostage to signal delivery: lose the PlaybackStatus=Playing edge and the poller stays down for the rest of the session, so the phone's now-playing freezes while audio plays. Live testing showed exactly this — armed at 05:04:37, stopped on pause at 05:05:43, never armed again despite playback resuming, and the drift broadcasts to the phone stopped with it. Firefox's MPRIS endpoint answers intermittently, so a missed edge is routine rather than a corner case, and chasing which signal path dropped it is not a good use of a release. A watchdog now samples every 10s and re-arms the poller when it finds unpolled playback, bounding the stale window regardless of signal reliability. It runs only while a local player is tracked, so a desktop with no MPRIS app keeps zero timers. Verified live: poller stopped 05:25:55 on pause, watchdog re-armed 05:26:33, with no kcd command issued in between. Also fixes a test-helper data race the shorter interval exposed: countingSender counted packets in a plain int while the plugin's real D-Bus watcher can discover a live player and broadcast concurrently. Trade-off worth stating plainly: with an MPRIS app open but paused the daemon now does one D-Bus read per 10s, so "zero timers at idle" becomes "zero timers when no MPRIS app is tracked". Playing remains 7.50 CPU ticks/min. --- internal/integration/pair_test.go | 9 +- internal/plugins/mpris/local.go | 8 +- internal/plugins/mpris/mpris.go | 10 ++- internal/plugins/mpris/playing_poller.go | 88 +++++++++++++++++++ internal/plugins/mpris/playing_poller_test.go | 42 +++++++++ internal/plugins/mpris/remote_gate_test.go | 22 +++-- 6 files changed, 162 insertions(+), 17 deletions(-) diff --git a/internal/integration/pair_test.go b/internal/integration/pair_test.go index 2617368..d91e616 100644 --- a/internal/integration/pair_test.go +++ b/internal/integration/pair_test.go @@ -207,17 +207,24 @@ func TestPairInitiateReturnsVerificationKeyIntegration(t *testing.T) { t.Fatalf("send identity: %v", err) } - deadline := time.Now().Add(3 * time.Second) + // Generous: under -race and a loaded CI runner the daemon's TLS + // handshake and device registration can take several seconds. + deadline := time.Now().Add(15 * time.Second) + registered := false for time.Now().Before(deadline) { devs, err := cl.Devices() if err != nil { t.Fatalf("list devices: %v", err) } if len(devs) > 0 && devs[0].ID == "mock-peer" { + registered = true break } time.Sleep(20 * time.Millisecond) } + if !registered { + t.Fatal("mock-peer never appeared in the device list") + } verificationKey, err := cl.Pair("mock-peer") if err != nil { diff --git a/internal/plugins/mpris/local.go b/internal/plugins/mpris/local.go index 68c08d6..a74bf04 100644 --- a/internal/plugins/mpris/local.go +++ b/internal/plugins/mpris/local.go @@ -123,9 +123,11 @@ func (p *MPRISPlugin) removePlayer(displayName string) { delete(p.players, displayName) delete(p.lastTracks, displayName) delete(p.lastStates, displayName) - // With no players left there is nothing to poll, so stop now rather - // than let the poller discover it on its next tick. sync handles - // the case where other players remain. + // With no players left there is nothing to poll, so stop the ticker + // and the watchdog rather than let them discover it on their own. + if len(p.players) == 0 { + p.stopWatchdogLocked() + } p.syncPlayingPollerLocked() p.mu.Unlock() diff --git a/internal/plugins/mpris/mpris.go b/internal/plugins/mpris/mpris.go index 5ad7a61..0252613 100644 --- a/internal/plugins/mpris/mpris.go +++ b/internal/plugins/mpris/mpris.go @@ -37,10 +37,12 @@ type MPRISPlugin struct { // ticker exists only while at least one local player IsPlaying. // pollCancel stops it; nil means no poller is running. pollGen // identifies the current poller so a self-stopping one does not - // clear its successor's handle. - mprisCfg config.MPRISConfig - pollCancel context.CancelFunc - pollGen uint64 + // clear its successor's handle. watchdogCancel stops the slow + // re-check that restarts a poller stranded by a missed signal. + mprisCfg config.MPRISConfig + pollCancel context.CancelFunc + pollGen uint64 + watchdogCancel context.CancelFunc // remotePollCancel stops the remote-state poller; nil means it is // not running. The poller is demand-driven (see syncRemotePoller): diff --git a/internal/plugins/mpris/playing_poller.go b/internal/plugins/mpris/playing_poller.go index f5496c0..a59005d 100644 --- a/internal/plugins/mpris/playing_poller.go +++ b/internal/plugins/mpris/playing_poller.go @@ -44,6 +44,77 @@ func (p *MPRISPlugin) syncPlayingPollerLocked() { return } p.armPlayingPollerLocked() + p.startWatchdogLocked() +} + +// startWatchdogLocked starts the slow re-check if it is not already +// running. Callers must hold p.mu. +func (p *MPRISPlugin) startWatchdogLocked() { + if p.watchdogCancel != nil { + return + } + ctx, cancel := context.WithCancel(p.watchCtx) + p.watchdogCancel = cancel + go p.runWatchdog(ctx) +} + +// stopWatchdogLocked stops the slow re-check. Callers must hold p.mu. +func (p *MPRISPlugin) stopWatchdogLocked() { + if p.watchdogCancel != nil { + p.watchdogCancel() + p.watchdogCancel = nil + } +} + +// runWatchdog restarts the position poller when it finds playback the +// signal path failed to announce. It stays parked — one read per +// interval — whenever the poller is already doing that job itself. +func (p *MPRISPlugin) runWatchdog(ctx context.Context) { + ticker := time.NewTicker(watchdogInterval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + p.mu.RLock() + armed := p.pollCancel != nil + tracked := len(p.players) + p.mu.RUnlock() + + if armed || tracked == 0 { + continue + } + if !p.anyPlayerPlaying() { + continue + } + p.logger.Debug("mpris: watchdog found unpolled playback, arming position poller") + p.mu.Lock() + p.armPlayingPollerLocked() + p.mu.Unlock() + } + } +} + +// anyPlayerPlaying reports whether any tracked player is playing right +// now. It reads live state without touching the cache or broadcasting — +// this only decides whether to restart the poller. +func (p *MPRISPlugin) anyPlayerPlaying() bool { + p.mu.RLock() + players := make([]*trackedPlayer, 0, len(p.players)) + for _, pl := range p.players { + players = append(players, pl) + } + p.mu.RUnlock() + + for _, pl := range players { + state, err := p.playerState(pl.displayName) + if err == nil && state.IsPlaying { + return true + } + } + return false } // armPlayingPollerLocked starts the position ticker if it is not already @@ -80,6 +151,23 @@ func (p *MPRISPlugin) stopPlayingPollerLocked() { // backstop for a name that lingers with a dead object behind it. const maxConsecutiveReadFailures = 5 +// watchdogInterval is how often the slow re-check looks for playback that +// the signal path missed. +// +// The poller stops the moment a live read reports nothing playing, and it +// only restarts when an observed change re-arms it. That makes it hostage +// to signal delivery: lose the PlaybackStatus=Playing edge and the poller +// stays down for the rest of the session, so the phone's now-playing +// freezes even though audio is playing. Firefox's MPRIS endpoint answers +// intermittently, which makes that a routine event, not a corner case. +// +// So the watchdog samples once per interval and restarts the poller if it +// finds something playing. 10s bounds how stale the phone's now-playing +// can get after a resume, at six reads per minute while an MPRIS app sits +// paused — still far below the 30/min the poller itself costs while +// playing, and it disappears entirely once no player is tracked. +const watchdogInterval = 10 * time.Second + // runPlayingPoller re-reads D-Bus state for every tracked player at // PositionInterval. It returns once live reads say nothing is playing, so // its lifetime follows the player rather than the cache that armed it. diff --git a/internal/plugins/mpris/playing_poller_test.go b/internal/plugins/mpris/playing_poller_test.go index 4e37b94..d709c2b 100644 --- a/internal/plugins/mpris/playing_poller_test.go +++ b/internal/plugins/mpris/playing_poller_test.go @@ -148,6 +148,48 @@ func TestPollDisarmsOnPlayerRemoval(t *testing.T) { } } +// The watchdog exists because signal delivery is unreliable: the poller +// stops on a confirmed pause and must be able to come back on its own +// when playback resumes. It starts with the first tracked player and is +// torn down with the last, so a desktop with no MPRIS app keeps no +// timers at all. +func TestWatchdogLifecycleFollowsTrackedPlayers(t *testing.T) { + p := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, testMPRISConfig(), log.Nop()) + defer p.watchCancel() + + watchdogRunning := func() bool { + p.mu.RLock() + defer p.mu.RUnlock() + return p.watchdogCancel != nil + } + + if watchdogRunning() { + t.Fatal("watchdog running with zero tracked players") + } + + trackPlayer(p, "Nightdrive") + p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: false}) + if !watchdogRunning() { + t.Fatal("watchdog not started alongside the first tracked player") + } + + trackPlayer(p, "Second FM") + p.storeLocalState("Second FM", &NowPlaying{Player: "Second FM", IsPlaying: false}) + if !watchdogRunning() { + t.Fatal("watchdog stopped while players remain") + } + + p.removePlayer("Nightdrive") + if !watchdogRunning() { + t.Fatal("watchdog stopped while one player remains") + } + + p.removePlayer("Second FM") + if watchdogRunning() { + t.Fatal("watchdog still running after the last player was removed") + } +} + // With PollWhilePlaying=false the poller must never arm (pure // event-driven mode). func TestPollNeverArmsWhenDisabled(t *testing.T) { diff --git a/internal/plugins/mpris/remote_gate_test.go b/internal/plugins/mpris/remote_gate_test.go index bb411e8..671c8d2 100644 --- a/internal/plugins/mpris/remote_gate_test.go +++ b/internal/plugins/mpris/remote_gate_test.go @@ -1,6 +1,7 @@ package mpris import ( + "sync/atomic" "testing" "github.com/bethropolis/kcd/internal/config" @@ -9,13 +10,16 @@ import ( "github.com/bethropolis/kcd/internal/protocol" ) +// countingSender counts packets atomically: the plugin's real D-Bus +// watcher can discover an actual player on the session bus and broadcast +// from its own goroutine while the test goroutine reads the count. type countingSender struct { testSender - sends int + sends atomic.Int32 } func (s *countingSender) Send(_ *protocol.Packet) error { - s.sends++ + s.sends.Add(1) return nil } @@ -38,8 +42,8 @@ func TestPollRemoteSilentWithoutSubscribers(t *testing.T) { p.pollRemoteStates() - if sender.sends != 0 { - t.Fatalf("sent %d requests with zero subscribers", sender.sends) + if sender.sends.Load() != 0 { + t.Fatalf("sent %d requests with zero subscribers", sender.sends.Load()) } } @@ -54,8 +58,8 @@ func TestPollRemoteRefreshesWhileWatched(t *testing.T) { p.pollRemoteStates() - if sender.sends != 1 { - t.Fatalf("sent %d requests while watched, want 1", sender.sends) + if sender.sends.Load() != 1 { + t.Fatalf("sent %d requests while watched, want 1", sender.sends.Load()) } } @@ -98,10 +102,10 @@ func TestPollRemoteSilentAfterUnsubscribe(t *testing.T) { sub := bus.Subscribe(4, events.TypeMprisUpdate) p.pollRemoteStates() sub.Close() - sender.sends = 0 + sender.sends.Store(0) p.pollRemoteStates() - if sender.sends != 0 { - t.Fatalf("sent %d requests after unsubscribe", sender.sends) + if sender.sends.Load() != 0 { + t.Fatalf("sent %d requests after unsubscribe", sender.sends.Load()) } } From 96aa1d22a0e06ca8adee758b63c39f87207c6073 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 25 Sep 2026 05:34:40 +0300 Subject: [PATCH 25/25] docs: state the MPRIS watchdog's cost instead of claiming zero timers The 10s watchdog added with the stranded-poller fix means an MPRIS player that is tracked but paused costs one D-Bus read per 10s. Every doc still promised "nothing is polled while paused" and "zero standing timers", which is no longer true on a desktop with a media player merely running. ARCHITECTURE.md gains a dedicated subsection for the one deliberate exception and explains why it exists, the contributor invariant now covers self-healing checks, and the anchor, CLIENT_GUIDE, and example config all describe the real behaviour. --- docs/ARCHITECTURE.md | 17 +++++++++++++---- docs/CLIENT_GUIDE.md | 7 +++++-- packaging/kcd.example.toml | 13 ++++++++----- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 7c595cb..e831e6d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -56,7 +56,7 @@ Devices are found via two parallel mechanisms that run concurrently: `Listener` binds to `0.0.0.0:1716` and parses every incoming UDP packet. Packets whose `type` is not `kdeconnect.identity` or whose `deviceId` matches the local device are silently dropped. -The broadcast interval is adaptive: when `shouldReduce()` returns true (all known devices already connected) the interval steps up to 60 seconds. Broadcast is controlled by a `BroadcasterController` which is off by default — it runs only while an owner holds it (`kcd pair` listen mode, or the reconnect watcher while any paired device is offline) and stops fully when the last owner withdraws, so connected steady state keeps zero timers. The UDP **listener** is always active. See [Idle behavior](#idle-behavior-zero-standing-timers) for the full zero-timer inventory. +The broadcast interval is adaptive: when `shouldReduce()` returns true (all known devices already connected) the interval steps up to 60 seconds. Broadcast is controlled by a `BroadcasterController` which is off by default — it runs only while an owner holds it (`kcd pair` listen mode, or the reconnect watcher while any paired device is offline) and stops fully when the last owner withdraws, so connected steady state keeps zero timers. The UDP **listener** is always active. See [Idle behavior](#idle-behavior-gated-timers) for the full zero-timer inventory. ### Ephemeral discovery dials @@ -81,7 +81,7 @@ At startup the `Broadcaster` registers the local device as a Zeroconf service wi --- -## Idle behavior (zero standing timers) +## Idle behavior (gated timers) Connected steady state (all pairs connected, nothing playing, no transfers, no pairing) keeps **zero application timers**. Every periodic source is gated on actual activity instead of running free: @@ -92,13 +92,22 @@ Connected steady state (all pairs connected, nothing playing, no transfers, no p | mDNS advertise | Lifetime-on, responder-only (no timers) | | UDP/TCP/IPC listeners, D-Bus signals, bus subscriptions | Blocking waits, zero CPU until an event arrives | | Local position poller | Exists only while ≥1 local player `IsPlaying` (`[mpris] poll_while_playing`, `position_interval`); ticks re-broadcast only on metadata change or position drift >3s off the anchor extrapolation | +| MPRIS watchdog | 10s re-check, alive only while ≥1 local player is tracked; restarts the position poller if it finds unpolled playback (see below) | | Remote state poller | Ticker itself exists only while a client subscribes to `mpris.update` (bus subscriber-change hook starts/stops it) | | Reconnect redial | Parked on discovery sightings; fallback escalates to `fallback_max`, then gives up past `stale_after` until the next sighting | | TCP keepalive | Kernel probes, first delay `[network] keepalive_idle` (default 30s, minimum 10s) | -Measured 2026-09-22 (phone connected, zero local players, one watch subscriber): 2 CPU ticks/min, 0 D-Bus `GetAll`/min — down from 13 ticks/min + 30 `GetAll`/min before. Methodology note: Go timers are runtime-managed (no timerfds to count) and `ptrace` is restricted by Yama, so `/proc` CPU deltas + `dbus-monitor` call rates are the working proxies. +### The one deliberate exception: the MPRIS watchdog -**Contributor invariant: new periodic work must be owner-gated or activity-gated, never standing.** A ticker that fires while nothing is happening is a bug — gate it on owners (discovery), playback state (MPRIS), subscribers (remote refresh), or sightings (reconnect). +The position poller arms on an observed state change and stops as soon as a live read confirms nothing is playing. Restarting it therefore depends on a D-Bus `PlaybackStatus` signal arriving — and a missed signal strands the poller for the rest of the session, leaving the phone's now-playing frozen while audio plays. Firefox's MPRIS endpoint answers intermittently, so a dropped edge is routine rather than a corner case. + +So while any local player is tracked, a 10s watchdog samples live state and re-arms the poller if it finds unpolled playback. This bounds the stale window regardless of signal reliability. + +The cost: with an MPRIS application open but paused, the daemon performs one D-Bus read per 10s. **"Zero timers at idle" means zero when no MPRIS player is tracked**, not zero on a desktop with a media player merely running. This is a deliberate trade — a bounded ~6 reads/min beats an unbounded frozen now-playing display. + +Measured 2026-09-25 (phone connected, Firefox playing): 7.5 CPU ticks/min, 0 voluntary context switches. Idle with no MPRIS player tracked: 0.00 CPU ticks/min, 0 `GetAll`/min. Methodology note: Go timers are runtime-managed (no timerfds to count) and `ptrace` is restricted by Yama, so `/proc` CPU deltas + `dbus-monitor` call rates are the working proxies. + +**Contributor invariant: new periodic work must be owner-gated or activity-gated, never standing.** A ticker that fires while nothing is happening is a bug — gate it on owners (discovery), playback state (MPRIS), subscribers (remote refresh), or sightings (reconnect). Where a signal-driven design cannot be made reliable, add a slow self-healing check scoped to the thing it watches, and document it here as a known cost rather than quietly reintroducing a standing timer. --- diff --git a/docs/CLIENT_GUIDE.md b/docs/CLIENT_GUIDE.md index 7850803..ab96150 100644 --- a/docs/CLIENT_GUIDE.md +++ b/docs/CLIENT_GUIDE.md @@ -324,8 +324,11 @@ except KeyboardInterrupt: > stays silent — the phone extrapolates from `posAnchorMs` — and a tick > re-broadcasts only on a metadata change or when the true position drifts > more than 3s off the extrapolation (seek, missed signal, clock drift). -> Nothing is polled while paused or with -> no players — a silent desktop costs zero wakeups. Set +> With no player running at all, nothing is polled — a silent desktop costs +> zero wakeups. While a player is merely paused, a 10s watchdog re-checks +> live state and restarts the poller if playback resumed without the daemon +> seeing the signal, so a dropped D-Bus edge cannot leave the phone's +> display frozen. Set > `poll_while_playing = false` for pure event-driven mode (position then > extrapolates from `posAnchorMs` between D-Bus signals). diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index e6b8af0..f3a4c46 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -192,11 +192,14 @@ remotesystemvolume = true [mpris] # The local D-Bus watcher is event-driven; this section tunes the position # poller that keeps the phone's now-playing display exact while music plays. -# poll_while_playing = true # false = pure event-driven (zero timers ever; - # position extrapolates from posAnchorMs, buggy - # silent players may go stale until next signal) -# position_interval = "2s" # D-Bus re-read cadence while playing only; - # nothing is polled while paused or with no players +# poll_while_playing = true # false = pure event-driven (position extrapolates + # from posAnchorMs; no poller and no watchdog, so a + # dropped D-Bus signal can leave the display stale) +# position_interval = "2s" # D-Bus re-read cadence while playing only. + # While a player is tracked but paused, a 10s + # watchdog still runs so a missed play signal cannot + # strand the poller; with no player tracked, zero + # timers remain. [cache] # Empty values preserve existing storage paths; use absolute paths to override.