Change how automatic pinning is implemented

This commit re-works how we automatically "pin" runtimes that are
explicitly installed, to prevent them from being removed automatically.
In this implementation we do the update to the config as part of the
deploy, which has the following advantages:
(1) It ensures that there's never a confusing polkit prompt about
configuring the software installation when the user asked for a runtime
to be installed (https://github.com/flatpak/flatpak/issues/4200)
(2) It means we don't have to rely on the code on the error path of
flatpak_transaction_real_run() to un-pin the runtime in case something
went wrong with the installation, since we pin it almost atomically with
the deploy.

Fixes #4200

5 files changed, 63 insertions, 49 deletions

diff --git a/common/flatpak-dir-private.h b/common/flatpak-dir-private.h
index db160368..ac229dd7 100644
--- a/common/flatpak-dir-private.h
+++ b/common/flatpak-dir-private.h
@@ -216,7 +216,7 @@ typedef enum {
   FLATPAK_HELPER_DEPLOY_FLAGS_NO_INTERACTION = 1 << 4,
   FLATPAK_HELPER_DEPLOY_FLAGS_APP_HINT = 1 << 5,
   FLATPAK_HELPER_DEPLOY_FLAGS_INSTALL_HINT = 1 << 6,
-  FLATPAK_HELPER_DEPLOY_FLAGS,
+  FLATPAK_HELPER_DEPLOY_FLAGS_UPDATE_PINNED = 1 << 7,
 } FlatpakHelperDeployFlags;
 
 #define FLATPAK_HELPER_DEPLOY_FLAGS_ALL (FLATPAK_HELPER_DEPLOY_FLAGS_UPDATE | \
@@ -225,7 +225,8 @@ typedef enum {
                                          FLATPAK_HELPER_DEPLOY_FLAGS_REINSTALL | \
                                          FLATPAK_HELPER_DEPLOY_FLAGS_NO_INTERACTION | \
                                          FLATPAK_HELPER_DEPLOY_FLAGS_APP_HINT | \
-                                         FLATPAK_HELPER_DEPLOY_FLAGS_INSTALL_HINT)
+                                         FLATPAK_HELPER_DEPLOY_FLAGS_INSTALL_HINT | \
+                                         FLATPAK_HELPER_DEPLOY_FLAGS_UPDATE_PINNED)
 
 typedef enum {
   FLATPAK_HELPER_UNINSTALL_FLAGS_NONE = 0,
@@ -715,6 +716,7 @@ gboolean              flatpak_dir_deploy_install                            (Fla
                                                                              const char                   **subpaths,
                                                                              const char                   **previous_ids,
                                                                              gboolean                       reinstall,
+                                                                             gboolean                       pin_on_deploy,
                                                                              GCancellable                  *cancellable,
                                                                              GError                       **error);
 gboolean              flatpak_dir_install                                   (FlatpakDir                    *self,
@@ -723,6 +725,7 @@ gboolean              flatpak_dir_install                                   (Fla
                                                                              gboolean                       no_static_deltas,
                                                                              gboolean                       reinstall,
                                                                              gboolean                       app_hint,
+                                                                             gboolean                       pin_on_deploy,
                                                                              FlatpakRemoteState            *state,
                                                                              FlatpakDecomposed             *ref,
                                                                              const char                    *opt_commit,
diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
index f43d3a87..1307f222 100644
--- a/common/flatpak-dir.c
+++ b/common/flatpak-dir.c
@@ -8563,6 +8563,7 @@ flatpak_dir_deploy_install (FlatpakDir        *self,
                             const char       **subpaths,
                             const char       **previous_ids,
                             gboolean           reinstall,
+                            gboolean           pin_on_deploy,
                             GCancellable      *cancellable,
                             GError           **error)
 {
@@ -8660,6 +8661,14 @@ flatpak_dir_deploy_install (FlatpakDir        *self,
   if (!flatpak_dir_mark_changed (self, error))
     goto out;
 
+  /* Pin runtimes that are installed explicitly rather than pulled as
+   * dependencies so they are not automatically removed. */
+  if (pin_on_deploy &&
+      !flatpak_dir_config_append_pattern (self, "pinned",
+                                          flatpak_decomposed_get_ref (ref),
+                                          TRUE, NULL, error))
+    goto out;
+
   ret = TRUE;
 
   commit = flatpak_dir_read_active (self, ref, cancellable);
@@ -9064,6 +9073,7 @@ flatpak_dir_install (FlatpakDir          *self,
                      gboolean             no_static_deltas,
                      gboolean             reinstall,
                      gboolean             app_hint,
+                     gboolean             pin_on_deploy,
                      FlatpakRemoteState  *state,
                      FlatpakDecomposed   *ref,
                      const char          *opt_commit,
@@ -9273,6 +9283,9 @@ flatpak_dir_install (FlatpakDir          *self,
       if (app_hint)
         helper_flags |= FLATPAK_HELPER_DEPLOY_FLAGS_APP_HINT;
 
+      if (pin_on_deploy)
+        helper_flags |= FLATPAK_HELPER_DEPLOY_FLAGS_UPDATE_PINNED;
+
       helper_flags |= FLATPAK_HELPER_DEPLOY_FLAGS_INSTALL_HINT;
 
       if (!flatpak_dir_system_helper_call_deploy (self,
@@ -9302,7 +9315,8 @@ flatpak_dir_install (FlatpakDir          *self,
   if (!no_deploy)
     {
       if (!flatpak_dir_deploy_install (self, ref, state->remote_name, opt_subpaths,
-                                       opt_previous_ids, reinstall, cancellable, error))
+                                       opt_previous_ids, reinstall, pin_on_deploy,
+                                       cancellable, error))
         return FALSE;
     }
 
@@ -9579,7 +9593,7 @@ flatpak_dir_install_bundle (FlatpakDir         *self,
     }
   else
     {
-      if (!flatpak_dir_deploy_install (self, ref, remote, NULL, NULL, FALSE, cancellable, error))
+      if (!flatpak_dir_deploy_install (self, ref, remote, NULL, NULL, FALSE, FALSE, cancellable, error))
         return FALSE;
     }
 
diff --git a/common/flatpak-installation.c b/common/flatpak-installation.c
index 0d5a3c10..aa11ebec 100644
--- a/common/flatpak-installation.c
+++ b/common/flatpak-installation.c
@@ -1926,7 +1926,7 @@ flatpak_installation_install_full (FlatpakInstallation    *self,
                             (flags & FLATPAK_INSTALL_FLAGS_NO_PULL) != 0,
                             (flags & FLATPAK_INSTALL_FLAGS_NO_DEPLOY) != 0,
                             (flags & FLATPAK_INSTALL_FLAGS_NO_STATIC_DELTAS) != 0,
-                            FALSE, FALSE, state,
+                            FALSE, FALSE, FALSE, state,
                             ref, NULL, (const char **) subpaths, NULL, NULL, NULL, NULL,
                             progress, cancellable, error))
     return NULL;
diff --git a/common/flatpak-transaction.c b/common/flatpak-transaction.c
index 418fa96b..551da495 100644
--- a/common/flatpak-transaction.c
+++ b/common/flatpak-transaction.c
@@ -107,6 +107,7 @@ struct _FlatpakTransactionOperation
   gboolean                        failed;
   gboolean                        skip;
   gboolean                        update_only_deploy;
+  gboolean                        pin_on_deploy;
 
   gboolean                        resolved;
   char                           *resolved_commit;
@@ -165,7 +166,6 @@ struct _FlatpakTransactionPrivate
   GPtrArray                   *extra_sideload_repos;
   GList                       *ops;
   GPtrArray                   *added_origin_remotes;
-  GPtrArray                   *added_pinned_runtimes;
 
   GList                       *flatpakrefs; /* GKeyFiles */
   GList                       *bundles; /* BundleData */
@@ -644,7 +644,8 @@ flatpak_transaction_operation_new (const char                     *remote,
                                    const char                    **previous_ids,
                                    const char                     *commit,
                                    GFile                          *bundle,
-                                   FlatpakTransactionOperationType kind)
+                                   FlatpakTransactionOperationType kind,
+                                   gboolean                        pin_on_deploy)
 {
   FlatpakTransactionOperation *self;
 
@@ -658,6 +659,7 @@ flatpak_transaction_operation_new (const char                     *remote,
   if (bundle)
     self->bundle = g_object_ref (bundle);
   self->kind = kind;
+  self->pin_on_deploy = pin_on_deploy;
 
   return self;
 }
@@ -1005,7 +1007,6 @@ flatpak_transaction_finalize (GObject *object)
   g_clear_object (&priv->dir);
 
   g_ptr_array_unref (priv->added_origin_remotes);
-  g_ptr_array_unref (priv->added_pinned_runtimes);
 
   g_ptr_array_free (priv->extra_dependency_dirs, TRUE);
   g_ptr_array_free (priv->extra_sideload_repos, TRUE);
@@ -1456,7 +1457,6 @@ flatpak_transaction_init (FlatpakTransaction *self)
   priv->last_op_for_ref = g_hash_table_new_full ((GHashFunc)flatpak_decomposed_hash, (GEqualFunc)flatpak_decomposed_equal, (GDestroyNotify) flatpak_decomposed_unref, NULL);
   priv->remote_states = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, (GDestroyNotify) flatpak_remote_state_unref);
   priv->added_origin_remotes = g_ptr_array_new_with_free_func (g_free);
-  priv->added_pinned_runtimes = g_ptr_array_new_with_free_func ((GDestroyNotify)flatpak_decomposed_unref);
   priv->extra_dependency_dirs = g_ptr_array_new_with_free_func (g_object_unref);
   priv->extra_sideload_repos = g_ptr_array_new_with_free_func (g_free);
   priv->can_run = TRUE;
@@ -1983,6 +1983,7 @@ flatpak_transaction_add_op (FlatpakTransaction             *self,
                             const char                     *commit,
                             GFile                          *bundle,
                             FlatpakTransactionOperationType kind,
+                            gboolean                        pin_on_deploy,
                             GError                        **error)
 {
   FlatpakTransactionPrivate *priv = flatpak_transaction_get_instance_private (self);
@@ -2012,7 +2013,8 @@ flatpak_transaction_add_op (FlatpakTransaction             *self,
       return op;
     }
 
-  op = flatpak_transaction_operation_new (remote, ref, subpaths, previous_ids, commit, bundle, kind);
+  op = flatpak_transaction_operation_new (remote, ref, subpaths, previous_ids,
+                                          commit, bundle, kind, pin_on_deploy);
   g_hash_table_insert (priv->last_op_for_ref, flatpak_decomposed_ref (ref), op);
 
   priv->ops = g_list_prepend (priv->ops, op);
@@ -2113,7 +2115,7 @@ add_related (FlatpakTransaction          *self,
           related_op = flatpak_transaction_add_op (self, rel->remote, rel->ref,
                                                    NULL, NULL, NULL, NULL,
                                                    FLATPAK_TRANSACTION_OPERATION_UNINSTALL,
-                                                   error);
+                                                   FALSE, error);
           if (related_op == NULL)
             return FALSE;
 
@@ -2137,7 +2139,7 @@ add_related (FlatpakTransaction          *self,
                                                    (const char **) rel->subpaths,
                                                    NULL, NULL, NULL,
                                                    FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE,
-                                                   error);
+                                                   FALSE, error);
           if (related_op == NULL)
             return FALSE;
 
@@ -2374,7 +2376,7 @@ add_deps (FlatpakTransaction          *self,
             return FALSE;
 
           runtime_op = flatpak_transaction_add_op (self, runtime_remote, runtime_ref, NULL, NULL, NULL, NULL,
-                                                   FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE, error);
+                                                   FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE, FALSE, error);
           if (runtime_op == NULL)
             return FALSE;
         }
@@ -2385,7 +2387,7 @@ add_deps (FlatpakTransaction          *self,
             {
               g_debug ("Updating dependent runtime %s", flatpak_decomposed_get_pref (runtime_ref));
               runtime_op = flatpak_transaction_add_op (self, runtime_remote, runtime_ref, NULL, NULL, NULL, NULL,
-                                                       FLATPAK_TRANSACTION_OPERATION_UPDATE, error);
+                                                       FLATPAK_TRANSACTION_OPERATION_UPDATE, FALSE, error);
               if (runtime_op == NULL)
                 return FALSE;
               runtime_op->non_fatal = TRUE;
@@ -2419,6 +2421,7 @@ flatpak_transaction_add_ref (FlatpakTransaction             *self,
                              FlatpakTransactionOperationType kind,
                              GFile                          *bundle,
                              const char                     *external_metadata,
+                             gboolean                        pin_on_deploy,
                              GError                        **error)
 {
   FlatpakTransactionPrivate *priv = flatpak_transaction_get_instance_private (self);
@@ -2530,7 +2533,8 @@ flatpak_transaction_add_ref (FlatpakTransaction             *self,
         return FALSE;
     }
 
-  op = flatpak_transaction_add_op (self, remote, ref, subpaths, previous_ids, commit, bundle, kind, error);
+  op = flatpak_transaction_add_op (self, remote, ref, subpaths, previous_ids,
+                                   commit, bundle, kind, pin_on_deploy, error);
   if (op == NULL)
     return FALSE;
 
@@ -2567,6 +2571,7 @@ flatpak_transaction_add_install (FlatpakTransaction *self,
   FlatpakTransactionPrivate *priv = flatpak_transaction_get_instance_private (self);
   g_autoptr(FlatpakDecomposed) decomposed = NULL;
   const char *all_paths[] = { NULL };
+  gboolean pin_on_deploy;
 
   g_return_val_if_fail (ref != NULL, FALSE);
   g_return_val_if_fail (remote != NULL, FALSE);
@@ -2579,24 +2584,12 @@ flatpak_transaction_add_install (FlatpakTransaction *self,
   if (subpaths == NULL)
     subpaths = all_paths;
 
-  if (!flatpak_transaction_add_ref (self, remote, decomposed, subpaths, NULL, NULL, FLATPAK_TRANSACTION_OPERATION_INSTALL, NULL, NULL, error))
-    return FALSE;
-
-  /* Pin runtimes that are installed explicitly rather than pulled as
-   * dependencies so they are not automatically removed. */
-  if (flatpak_decomposed_is_runtime (decomposed) && !priv->disable_auto_pin)
-    {
-      gboolean already_pinned;
+  pin_on_deploy = flatpak_decomposed_is_runtime (decomposed) && !priv->disable_auto_pin;
 
-      if (!flatpak_dir_config_append_pattern (priv->dir, "pinned", flatpak_decomposed_get_ref (decomposed), TRUE, &already_pinned, error))
-        return FALSE;
-
-      if (!already_pinned)
-        {
-          g_ptr_array_add (priv->added_pinned_runtimes, flatpak_decomposed_ref (decomposed));
-          flatpak_installation_drop_caches (priv->installation, NULL, NULL);
-        }
-    }
+  if (!flatpak_transaction_add_ref (self, remote, decomposed, subpaths, NULL, NULL,
+                                    FLATPAK_TRANSACTION_OPERATION_INSTALL,
+                                    NULL, NULL, pin_on_deploy, error))
+    return FALSE;
 
   return TRUE;
 }
@@ -2651,7 +2644,7 @@ flatpak_transaction_add_rebase (FlatpakTransaction *self,
   if (dir_ref_is_installed (priv->dir, decomposed, &installed_origin, NULL))
     remote = installed_origin;
 
-  return flatpak_transaction_add_ref (self, remote, decomposed, subpaths, previous_ids, NULL, FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE, NULL, NULL, error);
+  return flatpak_transaction_add_ref (self, remote, decomposed, subpaths, previous_ids, NULL, FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE, NULL, NULL, FALSE, error);
 }
 
 /**
@@ -2745,7 +2738,7 @@ flatpak_transaction_add_update (FlatpakTransaction *self,
     return FALSE;
 
   /* Note: we implement the merge when subpaths == NULL in flatpak_transaction_add_ref() */
-  return flatpak_transaction_add_ref (self, NULL, decomposed, subpaths, NULL, commit, FLATPAK_TRANSACTION_OPERATION_UPDATE, NULL, NULL, error);
+  return flatpak_transaction_add_ref (self, NULL, decomposed, subpaths, NULL, commit, FLATPAK_TRANSACTION_OPERATION_UPDATE, NULL, NULL, FALSE, error);
 }
 
 /**
@@ -2771,7 +2764,7 @@ flatpak_transaction_add_uninstall (FlatpakTransaction *self,
   if (decomposed == NULL)
     return FALSE;
 
-  return flatpak_transaction_add_ref (self, NULL, decomposed, NULL, NULL, NULL, FLATPAK_TRANSACTION_OPERATION_UNINSTALL, NULL, NULL, error);
+  return flatpak_transaction_add_ref (self, NULL, decomposed, NULL, NULL, NULL, FLATPAK_TRANSACTION_OPERATION_UNINSTALL, NULL, NULL, FALSE, error);
 }
 
 static gboolean
@@ -2883,7 +2876,7 @@ flatpak_transaction_add_auto_install (FlatpakTransaction *self,
 
                   if (!flatpak_transaction_add_ref (self, remote, auto_install_ref, NULL, NULL, NULL,
                                                     FLATPAK_TRANSACTION_OPERATION_INSTALL_OR_UPDATE,
-                                                    NULL, NULL,
+                                                    NULL, NULL, FALSE,
                                                     &local_error))
                     g_debug ("Failed to add auto-install ref %s: %s", flatpak_decomposed_get_ref (auto_install_ref),
                              local_error->message);
@@ -4266,7 +4259,7 @@ flatpak_transaction_resolve_bundles (FlatpakTransaction *self,
 
       if (!flatpak_transaction_add_ref (self, remote, ref, NULL, NULL, commit,
                                         FLATPAK_TRANSACTION_OPERATION_INSTALL_BUNDLE,
-                                        data->file, metadata, error))
+                                        data->file, metadata, FALSE, error))
         return FALSE;
     }
 
@@ -4308,6 +4301,7 @@ _run_op_kind (FlatpakTransaction           *self,
               FlatpakRemoteState           *remote_state, /* nullable */
               gboolean                     *out_needs_prune,
               gboolean                     *out_needs_triggers,
+              gboolean                     *out_needs_cache_drop,
               GCancellable                 *cancellable,
               GError                      **error)
 {
@@ -4336,6 +4330,7 @@ _run_op_kind (FlatpakTransaction           *self,
                                    priv->disable_static_deltas,
                                    priv->reinstall,
                                    priv->max_op >= APP_UPDATE,
+                                   op->pin_on_deploy,
                                    remote_state, op->ref,
                                    op->resolved_commit,
                                    (const char **) op->subpaths,
@@ -4373,6 +4368,9 @@ _run_op_kind (FlatpakTransaction           *self,
 
           if (flatpak_decomposed_is_app (op->ref))
             *out_needs_triggers = TRUE;
+
+          if (op->pin_on_deploy)
+            *out_needs_cache_drop = TRUE;
         }
     }
   else if (op->kind == FLATPAK_TRANSACTION_OPERATION_UPDATE)
@@ -4648,7 +4646,7 @@ add_uninstall_unused_ops (FlatpakTransaction  *self,
           uninstall_op = flatpak_transaction_add_op (self, origin, unused_ref,
                                                      NULL, NULL, NULL, NULL,
                                                      FLATPAK_TRANSACTION_OPERATION_UNINSTALL,
-                                                     NULL);
+                                                     FALSE, NULL);
           if (uninstall_op)
             run_operation_last (uninstall_op);
         }
@@ -4667,6 +4665,7 @@ flatpak_transaction_real_run (FlatpakTransaction *self,
   gboolean succeeded = TRUE;
   gboolean needs_prune = FALSE;
   gboolean needs_triggers = FALSE;
+  gboolean needs_cache_drop = FALSE;
   gboolean ready_res = FALSE;
   int i;
 
@@ -4826,7 +4825,9 @@ flatpak_transaction_real_run (FlatpakTransaction *self,
         }
 
       /* Here we execute the operation in a helper function */
-      if (res && !_run_op_kind (self, op, state, &needs_prune, &needs_triggers, cancellable, &local_error))
+      if (res && !_run_op_kind (self, op, state,
+                                &needs_prune, &needs_triggers, &needs_cache_drop,
+                                cancellable, &local_error))
         res = FALSE;
 
       if (res)
@@ -4885,15 +4886,9 @@ flatpak_transaction_real_run (FlatpakTransaction *self,
   for (i = 0; i < priv->added_origin_remotes->len; i++)
     flatpak_dir_prune_origin_remote (priv->dir, g_ptr_array_index (priv->added_origin_remotes, i));
 
-  for (i = 0; i < priv->added_pinned_runtimes->len; i++)
-    {
-      FlatpakDecomposed *pinned_runtime = g_ptr_array_index (priv->added_pinned_runtimes, i);
-      if (!dir_ref_is_installed (priv->dir, pinned_runtime, NULL, NULL))
-        {
-          flatpak_dir_config_remove_pattern (priv->dir, "pinned", flatpak_decomposed_get_ref (pinned_runtime), NULL);
-          flatpak_installation_drop_caches (priv->installation, NULL, NULL);
-        }
-    }
+  /* Reload config in case it changed via system helper */
+  if (needs_cache_drop || priv->added_origin_remotes->len > 0)
+    flatpak_installation_drop_caches (priv->installation, NULL, NULL);
 
   return succeeded;
 }
diff --git a/system-helper/flatpak-system-helper.c b/system-helper/flatpak-system-helper.c
index f83ecdf4..03410e84 100644
--- a/system-helper/flatpak-system-helper.c
+++ b/system-helper/flatpak-system-helper.c
@@ -399,6 +399,7 @@ handle_deploy (FlatpakSystemHelper   *object,
   gboolean no_deploy;
   gboolean local_pull;
   gboolean reinstall;
+  gboolean update_pinned;
   g_autofree char *url = NULL;
   g_autoptr(OngoingPull) ongoing_pull = NULL;
   g_autofree gchar *src_dir = NULL;
@@ -483,6 +484,7 @@ handle_deploy (FlatpakSystemHelper   *object,
   no_deploy = (arg_flags & FLATPAK_HELPER_DEPLOY_FLAGS_NO_DEPLOY) != 0;
   local_pull = (arg_flags & FLATPAK_HELPER_DEPLOY_FLAGS_LOCAL_PULL) != 0;
   reinstall = (arg_flags & FLATPAK_HELPER_DEPLOY_FLAGS_REINSTALL) != 0;
+  update_pinned = (arg_flags & FLATPAK_HELPER_DEPLOY_FLAGS_UPDATE_PINNED) != 0;
 
   deploy_dir = flatpak_dir_get_if_deployed (system, ref, NULL, NULL);
 
@@ -699,7 +701,7 @@ handle_deploy (FlatpakSystemHelper   *object,
           if (!flatpak_dir_deploy_install (system, ref, arg_origin,
                                            (const char **) arg_subpaths,
                                            (const char **) arg_previous_ids,
-                                           reinstall, NULL, &error))
+                                           reinstall, update_pinned, NULL, &error))
             {
               flatpak_invocation_return_error (invocation, error, "Error deploying");
               return G_DBUS_METHOD_INVOCATION_HANDLED;