Merge pull request 'The Decky plugin's "update the client" has never once detected an update' (#128) from worktree-decky-client-update into main

Reviewed-on: #128

clients/decky/main.py

@@ -303,6 +303,58 @@ def _native_client() -> str | None:
     return None
 
 
+# The one architecture the flatpak client is built for.
+_FLATPAK_ARCH = "x86_64"
+
+
+def _flatpak_ref() -> dict | None:
+    """The INSTALLED client flatpak resolved to a SCOPE and a BRANCH, or None when there is none.
+
+    ``{"scope": "--user"|"--system", "branch": "canary", "ref": "io.unom.Punktfunk//canary"}``.
+
+    ⭐⭐ **Naming no branch is not a shorthand for "the only one".** flatpak refuses an ambiguous
+    ref rather than guessing at one, and the ambiguity does not need two branches *installed*:
+    the punktfunk remote publishes `stable` AND `canary`, so an unqualified
+    ``flatpak remote-info <origin> io.unom.Punktfunk`` errors with "Multiple branches available"
+    on a Deck that has exactly one. That error is why the client update check silently answered
+    "up to date" on every Deck — so every query downstream now names the ref in full.
+
+    Read off the exported tree rather than by shelling out to ``flatpak list``, because
+    :func:`_client_argv` is on the path of every headless call and a subprocess per call would be
+    absurd (the same reason :func:`_flatpak_installed` reads the filesystem). ``active`` is the
+    symlink flatpak points at the deployed commit — its presence is what makes a branch directory
+    an INSTALL rather than the leftovers of one.
+
+    With more than one branch installed, `stable` wins, because that is the branch a plain
+    ``flatpak run`` resolves to: the check has to describe the client the launcher really starts,
+    or a stale `stable` silently beats a current `canary` in both places at once.
+    """
+    if not _flatpak():
+        return None
+    for root, scope in (
+        (Path(decky.DECKY_USER_HOME) / ".local" / "share" / "flatpak", "--user"),
+        (Path("/var/lib/flatpak"), "--system"),
+    ):
+        try:
+            branches = sorted(
+                p.name for p in (root / "app" / APP_ID / _FLATPAK_ARCH).iterdir()
+                if (p / "active").exists()
+            )
+        except OSError:
+            continue  # not installed in this scope
+        if not branches:
+            continue
+        branch = "stable" if "stable" in branches else branches[0]
+        if len(branches) > 1:
+            decky.logger.warning(
+                "%s is installed on %d branches (%s) — using %s, the one `flatpak run` resolves "
+                "to; uninstall the others so the client you launch is the client we update",
+                APP_ID, len(branches), ", ".join(branches), branch,
+            )
+        return {"scope": scope, "branch": branch, "ref": f"{APP_ID}//{branch}"}
+    return None
+
+
 def _flatpak_installed() -> bool:
     """True when the flatpak APP is actually installed — not merely that `flatpak` exists.
 
@@ -310,10 +362,7 @@ def _flatpak_installed() -> bool:
     because this is on the path of every headless call and a subprocess per call would be absurd.
     Both scopes count: the Deck installs --user, a distro image may ship it system-wide.
     """
-    if not _flatpak():
-        return False
-    user = Path(decky.DECKY_USER_HOME) / ".local" / "share" / "flatpak" / "app" / APP_ID
-    return user.exists() or Path("/var/lib/flatpak/app", APP_ID).exists()
+    return _flatpak_ref() is not None
 
 
 def _client_argv() -> list[str] | None:
@@ -323,15 +372,21 @@ def _client_argv() -> list[str] | None:
     behaving exactly as it did. A native binary is the fallback — and on a machine with no
     flatpak client, the thing that makes the plugin work at all. `PF_DECKY_CLIENT=native|flatpak`
     forces one when a machine has both.
+
+    The branch is PINNED (`--branch=`, which keeps the app id last — :func:`_cli_argv` appends
+    `--command=` and flatpak treats everything after the id as the app's own argv), so the client
+    this launches is the exact ref :func:`_client_update_state` checks and :meth:`Plugin.
+    update_client` updates.
     """
     forced = os.environ.get("PF_DECKY_CLIENT", "").strip().lower()
     native = _native_client()
     if forced == "native":
         return [native] if native else None
-    if forced != "flatpak" and not _flatpak_installed() and native:
+    ref = _flatpak_ref()
+    if forced != "flatpak" and not ref and native:
         return [native]
-    if _flatpak_installed():
-        return [_flatpak(), "run", "--arch=x86_64", APP_ID]
+    if ref:
+        return [_flatpak(), "run", f"--arch={_FLATPAK_ARCH}", f"--branch={ref['branch']}", APP_ID]
     return [native] if native else None
 
 
@@ -575,27 +630,44 @@ def _looks_outdated(stderr: str) -> bool:
 
 
 async def _client_update_state() -> dict:
-    """Is a newer commit of the flatpak client available in the remote it tracks? The client is a
-    **per-user** install (so ``sudo flatpak update``, which is system-scope, never touches it), and
-    it versions independently of this plugin — so we compare the installed commit against the
-    remote's here and let the QAM offer a user-scope update. Best-effort; all-``False`` on any error
-    (not installed, no flatpak, offline).
+    """Is a newer commit of the flatpak client available in the remote it tracks? The client
+    versions independently of this plugin, so we compare the installed commit against the
+    remote's here and let the QAM offer an update in the scope the client is actually installed
+    in — a per-user install is one ``sudo flatpak update`` (system-scope) never reaches.
 
     Flatpak keeps its OWN comparison (commits, not versions) because it is the exact one: a
     flatpak built from main between releases carries the release's crate version, so the
     signed-manifest comparison the native path uses would call it up to date when it isn't.
-    Native installs have no commit to compare and go through :func:`_native_update_state`."""
-    state = {"available": False, "installed": "", "remote": ""}
-    rc, info = await _flatpak_capture(["info", "--user", APP_ID], timeout=10.0)
+    Native installs have no commit to compare and go through :func:`_native_update_state`.
+
+    ⚠ Every query names the ref IN FULL (see :func:`_flatpak_ref`) — the remote publishes both
+    `stable` and `canary`, and an unqualified one is an error, not a default."""
+    state = {"available": False, "installed": "", "remote": "", "error": ""}
+    ref = _flatpak_ref()
+    if not ref:
+        return state  # no flatpak client in either scope
+    scope, full = ref["scope"], ref["ref"]
+    rc, info = await _flatpak_capture(["info", scope, full], timeout=10.0)
     if rc != 0:
-        return state  # client not installed as a user app / no flatpak
+        decky.logger.warning("flatpak info %s %s failed (rc=%s): %s", scope, full, rc, info[-200:])
+        state["error"] = "client-unavailable"
+        return state
     state["installed"] = _field_from(info, "Commit")
     origin = _field_from(info, "Origin")
     if not origin:
+        state["error"] = "no-origin"  # a sideloaded bundle tracks no remote to compare against
         return state
-    rc, rinfo = await _flatpak_capture(["remote-info", "--user", origin, APP_ID], timeout=25.0)
+    rc, rinfo = await _flatpak_capture(["remote-info", scope, origin, full], timeout=25.0)
     if rc != 0:
-        return state  # remote unreachable — treat as "up to date", retry next check
+        # ⭐ NOT "up to date". Silently swallowing this is precisely how the whole leg stayed
+        # broken in the field: an unqualified ref made every one of these calls fail, and
+        # returning `available=False` dressed the failure up as good news. A check that could
+        # not run says so, and the panel says so too.
+        decky.logger.warning(
+            "flatpak remote-info %s %s failed (rc=%s): %s", origin, full, rc, rinfo.strip()[-200:]
+        )
+        state["error"] = "fetch-failed"
+        return state
     state["remote"] = _field_from(rinfo, "Commit")
     state["available"] = bool(
         state["installed"] and state["remote"] and state["installed"] != state["remote"]
@@ -946,8 +1018,9 @@ class Plugin:
     async def update_client(self) -> dict:
         """Update the **client**, by whichever route this box's install actually supports.
 
-        * **flatpak** — ``flatpak update --user`` in the USER installation, the scope a Steam
-          Deck install lives in and which ``sudo flatpak update`` (system-scope) never reaches.
+        * **flatpak** — ``flatpak update`` against the FULL ref, in the scope the client is
+          installed in (a per-user install is one ``sudo flatpak update`` never reaches, and an
+          unqualified ref is an error on a remote publishing more than one branch).
         * **native, one-tap capable** (.deb / .rpm / pacman with the packaged root helper and
           the operator's group opt-in) — ``punktfunk-client --apply-update``, which starts the
           fixed, parameterless ``punktfunk-client-update.service`` through polkit. This backend
@@ -960,18 +1033,22 @@ class Plugin:
         """
         if not _client_is_flatpak():
             return await self._update_native_client()
-        _, before = await _flatpak_capture(["info", "--user", APP_ID], timeout=10.0)
+        ref = _flatpak_ref()
+        if not ref:
+            return {"ok": False, "updated": False, "error": "client-unavailable"}
+        scope, full = ref["scope"], ref["ref"]
+        _, before = await _flatpak_capture(["info", scope, full], timeout=10.0)
         before_commit = _field_from(before, "Commit")
-        rc, out = await _flatpak_capture(["update", "--user", "-y", APP_ID], timeout=300.0)
+        rc, out = await _flatpak_capture(["update", scope, "-y", full], timeout=300.0)
         if rc != 0:
             decky.logger.warning("flatpak client update failed (rc=%s): %s", rc, out[-400:])
             return {"ok": False, "updated": False, "error": "update-failed"}
-        _, after = await _flatpak_capture(["info", "--user", APP_ID], timeout=10.0)
+        _, after = await _flatpak_capture(["info", scope, full], timeout=10.0)
         after_commit = _field_from(after, "Commit")
         updated = bool(before_commit and after_commit and before_commit != after_commit)
         decky.logger.info(
-            "flatpak client update: %s -> %s (updated=%s)",
-            before_commit[:10], after_commit[:10], updated,
+            "flatpak client update (%s %s): %s -> %s (updated=%s)",
+            scope, full, before_commit[:10], after_commit[:10], updated,
         )
         _update_cache["data"] = None  # invalidate the cached "update available" snapshot
         return {"ok": True, "updated": updated}
@@ -1018,12 +1095,20 @@ class Plugin:
         try:
             if _client_is_flatpak():
                 cu = await _client_update_state()
+                ref = _flatpak_ref()
                 result["client_update_available"] = bool(cu["available"])
                 result["client_current"] = (cu["installed"] or "")[:10]
                 result["client_latest"] = (cu["remote"] or "")[:10]
                 result["client_install"] = "flatpak"
                 result["client_applier"] = "flatpak"
-                result["client_command"] = f"flatpak update --user {APP_ID}"
+                # The line a user could actually run — same scope, same full ref we use. The old
+                # unqualified one errored out ("Multiple branches available") when pasted, too.
+                result["client_command"] = (
+                    f"flatpak update {ref['scope']} -y {ref['ref']}" if ref else ""
+                )
+                if cu["error"]:
+                    # Same contract as the native leg: "couldn't tell" is never "up to date".
+                    result["client_error"] = cu["error"]
             else:
                 nu = await _native_update_state()
                 result["client_update_available"] = bool(nu.get("update_available"))

clients/decky/scripts/test-backend.py

@@ -30,6 +30,10 @@ sys.modules["decky"] = decky
 sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
 import main  # noqa: E402  (the plugin backend)
 
+# The argv fixtures below monkey-patch `_client_argv` to pin one install shape; the
+# _flatpak_ref block wants the REAL resolver back, so keep a handle on it.
+_real_client_argv = main._client_argv
+
 failures = 0
 
 
@@ -75,6 +79,60 @@ check("cli argv: native without a sibling CLI is None", main._cli_argv() is None
 (tmp / "punktfunk").write_text("")
 check("cli argv: native sibling found", main._cli_argv() == [str(tmp / "punktfunk")])
 
+# ---- _flatpak_ref: the branch must be NAMED, always ---------------------------------------
+#
+# The bug this exists to prevent: every client-update query used to name no branch, and the
+# punktfunk remote publishes `stable` AND `canary` — so `flatpak remote-info <origin>
+# io.unom.Punktfunk` failed with "Multiple branches available", the check swallowed the failure,
+# and the panel reported the client up to date forever. One branch INSTALLED is not enough to
+# make the query unambiguous; the ambiguity lives on the remote.
+shutil.rmtree("/tmp/pf-test-home", ignore_errors=True)
+_fp_root = Path("/tmp/pf-test-home/.local/share/flatpak/app/io.unom.Punktfunk/x86_64")
+main._flatpak = lambda: "/usr/bin/flatpak"
+main._client_argv = _real_client_argv  # undo the fixture patches above
+
+check("ref: nothing installed => None", main._flatpak_ref() is None)
+
+
+def _install_branch(name: str):
+    """A deployed branch: the `active` symlink is what distinguishes an install from leftovers."""
+    commit = _fp_root / name / "deadbeef"
+    commit.mkdir(parents=True, exist_ok=True)
+    (_fp_root / name / "active").symlink_to("deadbeef")
+
+
+(_fp_root / "canary").mkdir(parents=True, exist_ok=True)
+check("ref: a branch dir without `active` is leftovers, not an install", main._flatpak_ref() is None)
+
+_install_branch("canary")
+ref = main._flatpak_ref()
+check("ref: the single installed branch is used", ref == {
+    "scope": "--user", "branch": "canary", "ref": "io.unom.Punktfunk//canary",
+})
+check(
+    "ref: the launcher pins that branch, app id still LAST",
+    main._client_argv() == [
+        "/usr/bin/flatpak", "run", "--arch=x86_64", "--branch=canary", "io.unom.Punktfunk",
+    ],
+)
+# The pin must survive _cli_argv's rewrite, or the CLI runs a different build than the GUI.
+check(
+    "ref: --command= is inserted before the app id, keeping the pin",
+    main._cli_argv() == [
+        "/usr/bin/flatpak", "run", "--arch=x86_64", "--branch=canary",
+        "--command=punktfunk", "io.unom.Punktfunk",
+    ],
+)
+
+# Two installed: `stable` is what a plain `flatpak run` resolves to, so it must be what we
+# check and update too — otherwise a leftover stale `stable` wins the launch while `canary`
+# gets the update, and the two halves disagree about which client is even running.
+_install_branch("stable")
+check("ref: with both installed, stable wins (what `flatpak run` picks)",
+      main._flatpak_ref()["branch"] == "stable")
+
+shutil.rmtree("/tmp/pf-test-home", ignore_errors=True)
+
 # ---- _cli_error: the CLI's exit-code contract -------------------------------------------
 #
 # Exit 5 + `unknown command` is how a client too old for a verb announces itself — the ONE

clients/decky/src/backend.ts

@@ -120,7 +120,9 @@ export interface UpdateInfo {
   client_applier: string;
   client_command: string; // one copy-pastable line that updates this install by hand
   client_opt_in: string; // set when one-tap WOULD work after `usermod -aG punktfunk-update`
-  client_error?: string; // the client check couldn't complete (e.g. "client-outdated")
+  // The client check couldn't complete — NEVER rendered as "up to date". "client-outdated" |
+  // "client-unavailable" | "no-origin" | "fetch-failed" (flatpak: the remote was unreachable).
+  client_error?: string;
   error?: string; // "update-channel-unknown" (dev build) | "fetch-failed"
 }
 

clients/decky/src/index.tsx

@@ -33,6 +33,7 @@ import {
   applyUpdate,
   checkForUpdatesNow,
   clientUpdateIsManualOnly,
+  clientUpdateIsOneTap,
   hasUpdate,
   HostView,
   needsPair,
@@ -183,8 +184,11 @@ const QamPanel: FC = () => {
                 onClick={() => applyUpdate(update!, check)}
                 label={
                   update!.update_available
-                    ? `Plugin v${update!.current} → v${update!.latest}${
-                        update!.client_update_available ? " + client" : ""
+                    ? // "+ client" only when this tap will really install it. A manual-only
+                      // client rides along as a toast with the command, and promising it in the
+                      // label would make that read as a failure.
+                      `Plugin v${update!.current} → v${update!.latest}${
+                        clientUpdateIsOneTap(update) ? " + client" : ""
                       }`
                     : "New client version"
                 }