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 diff --git a/.goreleaser.yaml b/.goreleaser.yaml index ec49bcc..31803d7 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" @@ -262,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 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/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 e757cf1..90fc91f 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" ) @@ -14,16 +15,76 @@ 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) } } +// 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 := actionDeviceID(c) + 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)", @@ -143,61 +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 (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", + 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 (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"), "Previous"); err != nil { - return err - } - return cl.MprisAction(c.String("device"), c.String("player"), "Play") - }, + 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_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/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/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_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/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/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 22339f8..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 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-gated-timers) for the full zero-timer inventory. ### Ephemeral discovery dials @@ -75,12 +75,42 @@ 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. --- +## 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: + +| 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`); 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) | + +### The one deliberate exception: the MPRIS watchdog + +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. + +--- + ## 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. @@ -357,7 +387,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/docs/CLI.md b/docs/CLI.md index f3db347..0f30242 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -29,9 +29,10 @@ 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"` | +| `[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"` | | `[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 | @@ -53,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"`). --- @@ -200,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). @@ -234,6 +259,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 @@ -291,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] ``` --- @@ -301,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** @@ -408,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** @@ -423,26 +450,26 @@ 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 -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 ] -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 @@ -450,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 @@ -532,7 +559,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 @@ -631,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. --- @@ -778,12 +805,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. @@ -864,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 @@ -873,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 @@ -882,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] ``` --- @@ -896,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** diff --git a/docs/CLIENT_GUIDE.md b/docs/CLIENT_GUIDE.md index 9f01f3c..ab96150 100644 --- a/docs/CLIENT_GUIDE.md +++ b/docs/CLIENT_GUIDE.md @@ -294,11 +294,14 @@ 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 -> 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 +> **Freshness:** while at least one client watches `mpris.update`, the +> 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 > from its `playerList` (session destroyed) the cached state is dropped with @@ -312,7 +315,22 @@ 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). 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). +> 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). See [`IPC_PROTOCOL.md §5`](IPC_PROTOCOL.md#5-event-types) for the full list. diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index ec4088b..6700f9f 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` @@ -490,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` @@ -781,7 +800,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 +1260,12 @@ 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 -> device with an **actively-playing** player every 5 seconds -> (`kdeconnect.mpris.request` with `requestNowPlaying: true`). Responses are +> **Freshness:** while at least one client subscribes to `mpris.update`, +> 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 @@ -1366,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/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" ]; diff --git a/internal/config/config.go b/internal/config/config.go index dcc3641..03df368 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"` @@ -68,13 +69,14 @@ func Defaults() *Config { c.TCPPort = protocol.DefaultTCPPort c.LogLevel = "info" - c.Network = NetworkConfig{DialTimeout: "5s", HandshakeTimeout: "10s", SidechannelTimeout: "15s", TransferIdleTimeout: "60s"} - c.Reconnect = ReconnectConfig{InitialBackoff: "2s", MaxBackoff: "5m", FlapThreshold: "15s"} + 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() 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..87172b9 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -17,10 +17,10 @@ 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"}) { + 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", "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" { @@ -131,9 +134,12 @@ 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"}, {"pairing", "intent_ttl"}, {"pairing", "listen_timeout"}, + {"mpris", "position_interval"}, } for _, field := range fields { for _, value := range []string{"", "nonsense", "10", "0", "0s", "-1s", "9999999999999999999h"} { @@ -166,6 +172,40 @@ 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) + } + // 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) { + // 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 d3b0495..cf28cda 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. @@ -20,6 +26,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. @@ -28,6 +41,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"` @@ -51,13 +71,17 @@ 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}, + {"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}, {"pairing.listen_timeout", c.Pairing.ListenTimeout}, + {"mpris.position_interval", c.MPRIS.PositionInterval}, } { d, err := time.ParseDuration(setting.value) if err != nil { @@ -70,6 +94,12 @@ 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 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 { + 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/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/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/daemon/plugins.go b/internal/daemon/plugins.go index 358c09e..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) @@ -64,7 +65,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/daemon/transport.go b/internal/daemon/transport.go index 5cb25e4..b15e4d9 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. @@ -180,6 +181,21 @@ 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 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() + go reconnectWithBackoff(ctx, dev, ip, identity, cfg, devices, plugins, localDeviceID, logger, opts) + } + } return } @@ -217,6 +233,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/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/daemon/transport_reconnect.go b/internal/daemon/transport_reconnect.go index c1751ec..05cd2e2 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,35 +87,72 @@ 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 + } + + // 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), ) // Prefer the peer's last advertised listening port over the // 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() { 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..9b20060 --- /dev/null +++ b/internal/daemon/transport_reconnect_test.go @@ -0,0 +1,244 @@ +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) +} + +// 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) { + 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/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 e9131cf..15a2b78 100644 --- a/internal/discovery/broadcaster_controller.go +++ b/internal/discovery/broadcaster_controller.go @@ -30,7 +30,16 @@ 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. + browseStarter func(ctx context.Context) } // defaultIdleInterval is the broadcast period while idle (all pairs @@ -68,6 +77,20 @@ func (bc *BroadcasterController) Stop() { bc.StopOwned(OwnerPairing) } +// SetBrowseStarter registers the mDNS browse function to run alongside +// 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. func (bc *BroadcasterController) StartOwned(parentCtx context.Context, owner string) { bc.mu.Lock() @@ -81,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, @@ -95,6 +119,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..0957865 100644 --- a/internal/discovery/discovery_test.go +++ b/internal/discovery/discovery_test.go @@ -61,6 +61,81 @@ 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") + } +} + +// 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) + 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..fb7bce9 100644 --- a/internal/discovery/listener_mdns.go +++ b/internal/discovery/listener_mdns.go @@ -46,11 +46,18 @@ 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; zeroconf closes the entries channel, which ends the results loop. +func (l *Listener) RunMdnsDiscovery(ctx context.Context) { entries := make(chan *zeroconf.ServiceEntry) go func(results <-chan *zeroconf.ServiceEntry) { for entry := range results { diff --git a/internal/events/bus.go b/internal/events/bus.go index eee0029..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,19 +130,62 @@ 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() + } +} + +// 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. diff --git a/internal/events/bus_test.go b/internal/events/bus_test.go new file mode 100644 index 0000000..6526a9c --- /dev/null +++ b/internal/events/bus_test.go @@ -0,0 +1,60 @@ +package events + +import ( + "sync/atomic" + "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") + } +} + +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/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..d91e616 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,95 @@ 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) + } + + // 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 { + 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 c5de5eb..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"` @@ -201,6 +209,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/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 3f591db..a74bf04 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,7 +69,22 @@ func (p *MPRISPlugin) sendPlayerListBroadcast() { } func (p *MPRISPlugin) broadcast(state *NowPlaying) { - pkt, err := protocol.NewPacket("kdeconnect.mpris", state) + // 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. + // + // 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", &snapshot) if err != nil { return } @@ -95,9 +111,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 +123,12 @@ 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 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() p.logger.Debug("mpris: removed player", log.String("displayName", displayName)) @@ -164,6 +184,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..0252613 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,26 @@ 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. pollGen + // identifies the current poller so a self-stopping one does not + // 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): + // 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. @@ -60,7 +78,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,15 +103,21 @@ 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) - 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 43716b8..2c5a5b3 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,12 @@ func TestStampAlbumArtMatchesCurrentTrack(t *testing.T) { } func TestPollRemoteStatesOnlyTargetsKnownPlayers(t *testing.T) { - plugin := NewMPRISPlugin(nil, events.NewBus(log.Nop()), false, 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() } @@ -357,7 +363,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 +404,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 +435,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 +464,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() } @@ -503,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 new file mode 100644 index 0000000..a59005d --- /dev/null +++ b/internal/plugins/mpris/playing_poller.go @@ -0,0 +1,261 @@ +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 +// 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 + p.syncPlayingPollerLocked() + p.mu.Unlock() +} + +// 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 + } + if len(p.players) == 0 { + p.stopPlayingPollerLocked() + 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 +// 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. +// 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 + } +} + +// 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 + +// 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. +// 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: + 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 + } + } + } + } +} + +// 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 + } +} + +// pollPlayingPlayers samples every tracked player once, broadcasting the +// 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 { + players = append(players, pl) + } + p.mu.RUnlock() + + 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 { + 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, complete +} diff --git a/internal/plugins/mpris/playing_poller_test.go b/internal/plugins/mpris/playing_poller_test.go new file mode 100644 index 0000000..d709c2b --- /dev/null +++ b/internal/plugins/mpris/playing_poller_test.go @@ -0,0 +1,288 @@ +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"} +} + +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() + + trackPlayer(p, "Nightdrive") + if pollerArmed(p) { + t.Fatal("poller armed before any state was observed") + } + + // 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)") + } +} + +// 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") + } +} + +// 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() + + // 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") + } + + // 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 never gave up after the bounded failure backstop") + } +} + +// 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() + cleared := p.pollCancel == nil + p.mu.RUnlock() + 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 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() + + trackPlayer(p, "Nightdrive") + p.storeLocalState("Nightdrive", &NowPlaying{Player: "Nightdrive", IsPlaying: true}) + if !pollerArmed(p) { + t.Fatal("poller not armed before removal") + } + + p.removePlayer("Nightdrive") + + if pollerArmed(p) { + t.Fatal("poller still armed after last player removed") + } +} + +// 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) { + 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}) + + if pollerArmed(p) { + t.Fatal("poller armed with PollWhilePlaying=false") + } +} + +// localStateChanged is the shared compare behind the poller tick and the +// 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(now, base, nil) { + t.Fatal("nil cache must count as changed") + } + same := &NowPlaying{Player: "Nightdrive", PlaybackStatus: "Playing", IsPlaying: true, Title: "T"} + 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(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()) + defer p.watchCancel() + + 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/reconcile.go b/internal/plugins/mpris/reconcile.go index 5aa5471..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" ) @@ -52,6 +54,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 +107,81 @@ 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 + } + // The comparison reads the cached anchor, which broadcast + // stamps under p.mu — hold RLock through it. + p.mu.RLock() + changed := localStateChanged(time.Now().UnixMilli(), state, p.lastStates[display]) + p.mu.RUnlock() + 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. +// 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 || + state.Artist != last.Artist || + state.Album != last.Album || + state.AlbumArtUrl != last.AlbumArtUrl || + state.Volume != last.Volume || + 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 8c3a940..ff1d207 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" ) @@ -191,25 +192,49 @@ func (p *MPRISPlugin) requestPlayerListPeriodic(dev device.Sender) { p.requestPlayerList(dev) } -// 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. -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() - } +// 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: 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) 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 @@ -217,7 +242,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..671c8d2 --- /dev/null +++ b/internal/plugins/mpris/remote_gate_test.go @@ -0,0 +1,111 @@ +package mpris + +import ( + "sync/atomic" + "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" +) + +// 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 atomic.Int32 +} + +func (s *countingSender) Send(_ *protocol.Packet) error { + s.sends.Add(1) + 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.Load() != 0 { + t.Fatalf("sent %d requests with zero subscribers", sender.sends.Load()) + } +} + +// 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.Load() != 1 { + t.Fatalf("sent %d requests while watched, want 1", sender.sends.Load()) + } +} + +// 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) { + bus := events.NewBus(log.Nop()) + p, sender := playingRemotePlugin(t, bus) + + sub := bus.Subscribe(4, events.TypeMprisUpdate) + p.pollRemoteStates() + sub.Close() + sender.sends.Store(0) + p.pollRemoteStates() + + if sender.sends.Load() != 0 { + t.Fatalf("sent %d requests after unsubscribe", sender.sends.Load()) + } +} diff --git a/internal/plugins/mpris/signals.go b/internal/plugins/mpris/signals.go index d9b87c9..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 } @@ -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..23066b0 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 { @@ -96,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 { @@ -119,6 +70,10 @@ func (p *MPRISPlugin) runDBusWatcher(ctx context.Context) error { } } + // 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"), @@ -126,7 +81,9 @@ func (p *MPRISPlugin) runDBusWatcher(ctx context.Context) error { return err } - ch := make(chan *dbus.Signal, 64) + // Sized for a burst of player churn (browser restarts, several + // players appearing at once). + ch := make(chan *dbus.Signal, 256) conn.Signal(ch) for { 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/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/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/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 diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index 1a22b5d..f3a4c46 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -172,11 +172,16 @@ 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" # 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" @@ -184,6 +189,18 @@ 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 (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. # sms_attachments_dir = "" # default: system temp directory/kcd/sms-attachments 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 # --------------------------------------------------------------------------- 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. 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. 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}" diff --git a/scripts/install.sh b/scripts/install.sh index ae08d3a..7eb0ab7 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}+." @@ -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)" 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 "================================================================="