Clarify clear facts (#50667)

* Revert "avoid x2 setting of set_fact when 'cacheable' (#50564)"

This reverts commit 207848f354.

* clarify clear_facts with set_fact cacheable

 revert previous 'fix' as it will break playbooks by changing precedence
 opted to leave current behaviour but document it on both plugins to mitigate confusion

 fixes #50556

 also fix grammer, add comment, remove unused e

changelogs/fragments/fix_set_fact_cacheable.yml

@@ -1,2 +0,0 @@
-bugfixes:
-    - fix issue in which 'cacheable' was not being properly applied and the fact was x2 defined

docs/docsite/rst/user_guide/playbooks_variables.rst

@@ -1036,7 +1036,8 @@ Basically, anything that goes into "role defaults" (the defaults folder inside t
 .. [1] Tasks in each role will see their own role's defaults. Tasks defined outside of a role will see the last role's defaults.
 .. [2] Variables defined in inventory file or provided by dynamic inventory.
 .. [3] Includes vars added by 'vars plugins' as well as host_vars and group_vars which are added by the default vars plugin shipped with Ansible.
-.. [4] When created with set_facts's cacheable option.
+.. [4] When created with set_facts's cacheable option, variables will have the high precedence in the play,
+       but will be the same as a host facts precedence when they come from the cache.
 
 .. note:: Within any section, redefining a var will overwrite the previous instance.
           If multiple groups have the same variable, the last one loaded wins.

lib/ansible/executor/task_executor.py

@@ -684,7 +684,7 @@ class TaskExecutor:
                 return failed_when_result
 
             if 'ansible_facts' in result:
-                if self._task.action in ('include_vars', 'set_fact'):
+                if self._task.action in ('set_fact', 'include_vars'):
                     vars_copy.update(result['ansible_facts'])
                 else:
                     # TODO: cleaning of facts should eventually become part of taskresults instead of vars

lib/ansible/modules/utilities/helper/meta.py

@@ -45,6 +45,7 @@ options:
 notes:
     - C(meta) is not really a module nor action_plugin as such it cannot be overwritten.
     - This module is also supported for Windows targets.
+    - "C(clear_facts) will remove the persistent facts from ``set_fact: cacheable=True``, but not the current host variable it creates for the current run."
 author:
     - "Ansible Core Team"
 '''

lib/ansible/modules/utilities/logic/set_fact.py

@@ -40,6 +40,9 @@ options:
       - Normally this module creates 'host level variables' and has much higher precedence, this option changes the nature and precedence
         (by 7 steps) of the variable created.
         https://docs.ansible.com/ansible/latest/user_guide/playbooks_variables.html#variable-precedence-where-should-i-put-a-variable
+      - "This actually creates 2 copies of the variable, a normal 'set_fact' host variable with high precedence and
+        a lower 'ansible_fact' one that is available for persistance via the facts cache plugin.
+        This creates a possibly confusing interaction with ``meta: clear_facts`` as it will remove the 'ansible_fact' but not the host variable."
     type: bool
     default: 'no'
     version_added: "2.4"

lib/ansible/plugins/strategy/__init__.py

@@ -570,10 +570,15 @@ class StrategyBase:
                         else:
                             cacheable = result_item.pop('_ansible_facts_cacheable', False)
                             for target_host in host_list:
-                                if original_task.action == 'set_fact' and not cacheable:
-                                    self._variable_manager.set_nonpersistent_facts(target_host, result_item['ansible_facts'].copy())
-                                else:
+                                # so set_fact is a misnomer but 'cacheable = true' was meant to create an 'actual fact'
+                                # to avoid issues with precedence and confusion with set_fact normal operation,
+                                # we set BOTH fact and nonpersistent_facts (aka hostvar)
+                                # when fact is retrieved from cache in subsequent operations it will have the lower precedence,
+                                # but for playbook setting it the 'higher' precedence is kept
+                                if original_task.action != 'set_fact' or cacheable:
                                     self._variable_manager.set_host_facts(target_host, result_item['ansible_facts'].copy())
+                                if original_task.action == 'set_fact':
+                                    self._variable_manager.set_nonpersistent_facts(target_host, result_item['ansible_facts'].copy())
 
                     if 'ansible_stats' in result_item and 'data' in result_item['ansible_stats'] and result_item['ansible_stats']['data']:
 
@@ -892,7 +897,7 @@ class StrategyBase:
                 loader=self._loader,
                 variable_manager=self._variable_manager
             )
-        except AnsibleError as e:
+        except AnsibleError:
             return False
 
         result = True