CI pipeline reliability: netapi key-leak, event tagger, and docs dedup fixes (#69728, #69730, #69724)

changelog/69724.fixed.md

@@ -0,0 +1 @@
+Fixed the intermittent ``duplicate HTTP post method definition`` failure in the -W parallel docs builds (Prepare Release and Documentation jobs) by marking the HTTP routes documented on the rest_tornado and rest_wsgi pages with ``:noindex:``, leaving rest_cherrypy as the single indexed instance of each shared route.

changelog/69728.fixed.md

@@ -0,0 +1 @@
+Fixed the Rocky Linux 9 integration tcp/zeromq CI jobs failing most PR runs: the startup_states and salt_call ownership test fixtures left their extra minions' accepted keys on the shared session master after stopping the minions, so later netapi tests targeting ``*`` matched dead minions (wrong minion lists and 30 second timeouts). The fixtures now delete their minion keys at teardown.

changelog/69730.fixed.md

@@ -0,0 +1 @@
+Fixed the master logging ``Event iteration failed with exception: 'list' object has no attribute 'items'`` for every failing state compilation: the return of a failed compile is a list of error strings, not a mapping of state results, and the event tagger assumed a dict.

changelog/69738.fixed.md

@@ -0,0 +1 @@
+Fixed the flaky ssh test_renderer_file: salt-ssh slsutil.renderer does not ship a rendered file's jinja imports (map.jinja) to the target, so the renderer tests only passed when an earlier state test had warmed the salt-ssh file cache. Prime the cache in the fixture so they are deterministic.

doc/_ext/salthttpanchors.py

@@ -0,0 +1,45 @@
+"""
+Keep HTML anchors on ``:noindex:``'d httpdomain directives.
+
+sphinxcontrib-httpdomain's ``add_target_and_index`` intentionally splits its
+two jobs: it always appends the ``#<method>--<path>`` anchor to the signature
+node, and only gates the global route *registration* behind ``:noindex:``.
+Sphinx's ``ObjectDescription.run`` however skips the whole method when
+``noindex`` is set, so the anchor (and its permalink) is lost along with the
+index entry. Anchors are per-page HTML ids and cannot collide across pages,
+so restoring them is safe; only the global registration can produce the
+parallel-build duplicate-route warnings.
+
+Hide the option from Sphinx's outer gate and re-present it to httpdomain's
+inner gate, so ``:noindex:`` means what httpdomain meant it to mean: no index
+entry, anchor kept.
+"""
+
+from sphinxcontrib.httpdomain import HTTPDomain
+
+
+def _make_anchored(cls):
+    class AnchoredHTTPResource(cls):
+        def run(self):
+            self._salt_noindex = "noindex" in self.options
+            self.options.pop("noindex", None)
+            return super().run()
+
+        def add_target_and_index(self, name_cls, sig, signode):
+            if self._salt_noindex:
+                self.options["noindex"] = None
+            try:
+                super().add_target_and_index(name_cls, sig, signode)
+            finally:
+                if self._salt_noindex:
+                    self.options.pop("noindex", None)
+
+    AnchoredHTTPResource.__name__ = f"Anchored{cls.__name__}"
+    return AnchoredHTTPResource
+
+
+def setup(app):
+    app.setup_extension("sphinxcontrib.httpdomain")
+    for name, cls in list(HTTPDomain.directives.items()):
+        app.add_directive_to_domain("http", name, _make_anchored(cls), override=True)
+    return {"parallel_read_safe": True, "parallel_write_safe": True}

doc/conf.py

@@ -176,6 +176,7 @@ def _safe_urlsplit(url, scheme="", allow_fragments=True):
     "sphinx.ext.imgconverter",
     "sphinx.ext.intersphinx",
     "sphinxcontrib.httpdomain",
+    "salthttpanchors",
     "saltrepo",
     "myst_parser",
     #'saltautodoc', # Must be AFTER autodoc

salt/netapi/rest_tornado/saltnado.py

@@ -633,6 +633,7 @@ def get(self):  # pylint: disable=arguments-differ
         All logins are done over post, this is a parked endpoint
 
         .. http:get:: /login
+            :noindex:
 
             :status 401: |401|
             :status 406: |406|
@@ -672,6 +673,7 @@ def post(self):  # pylint: disable=arguments-differ
         :ref:`Authenticate <rest_tornado-auth>` against Salt's eauth system
 
         .. http:post:: /login
+            :noindex:
 
             :reqheader X-Auth-Token: |req_token|
             :reqheader Accept: |req_accept|
@@ -803,6 +805,7 @@ def get(self):  # pylint: disable=arguments-differ
         An endpoint to determine salt-api capabilities
 
         .. http:get:: /
+            :noindex:
 
             :reqheader Accept: |req_accept|
 
@@ -841,6 +844,7 @@ def post(self):  # pylint: disable=arguments-differ
         Send one or more Salt commands (lowstates) in the request body
 
         .. http:post:: /
+            :noindex:
 
             :reqheader X-Auth-Token: |req_token|
             :reqheader Accept: |req_accept|
@@ -1219,6 +1223,7 @@ def get(self, mid=None):  # pylint: disable=W0221
         details
 
         .. http:get:: /minions/(mid)
+            :noindex:
 
             :reqheader X-Auth-Token: |req_token|
             :reqheader Accept: |req_accept|
@@ -1266,6 +1271,7 @@ def post(self):
         Start an execution command and immediately return the job id
 
         .. http:post:: /minions
+            :noindex:
 
             :reqheader X-Auth-Token: |req_token|
             :reqheader Accept: |req_accept|
@@ -1345,6 +1351,7 @@ def get(self, jid=None):  # pylint: disable=W0221
         the return from a single job
 
         .. http:get:: /jobs/(jid)
+            :noindex:
 
             List jobs or show a single job from the job cache.
 
@@ -1445,6 +1452,7 @@ def post(self):
         <rest_cherrypy-auth>`
 
         .. http:post:: /run
+            :noindex:
 
             This entry point is primarily for "one-off" commands. Each request
             must pass full Salt authentication credentials. Otherwise this URL
@@ -1519,6 +1527,7 @@ def get(self):
         event is formatted as JSON.
 
         .. http:get:: /events
+            :noindex:
 
             :status 200: |200|
             :status 401: |401|
@@ -1678,6 +1687,7 @@ def post(self, tag_suffix=None):  # pylint: disable=W0221
         Fire an event in Salt with a custom event tag and data
 
         .. http:post:: /hook
+            :noindex:
 
             :status 200: |200|
             :status 401: |401|

salt/netapi/rest_wsgi.py

@@ -65,6 +65,7 @@
 ==============
 
 .. http:post:: /
+    :noindex:
 
     **Example request** for a basic ``test.ping``::
 

salt/utils/event.py

@@ -937,6 +937,18 @@ def _fire_ret_load_specific_fun(self, load, fun_index=0):
             ret = load.get("return", {})
             retcode = load["retcode"]
 
+        if not isinstance(ret, dict):
+            # A failing state compilation returns a list of error strings (or
+            # a plain string from some renderers) instead of a mapping of
+            # per-state results, so there are no state tags to fire sub
+            # events for.
+            log.debug(
+                "Skipping sub event for job %s: return is a %s, not a dict",
+                load.get("jid"),
+                type(ret).__name__,
+            )
+            return
+
         try:
             for tag, data in ret.items():
                 data["retcode"] = retcode

tests/pytests/integration/cli/test_salt_call_ownership.py

@@ -64,6 +64,10 @@ def non_root_minion(salt_master, salt_factories):
     )
     with factory.started():
         yield factory
+    # The minion process is stopped at this point, but its accepted key stays
+    # on the shared session master, where later tests that target '*' (the
+    # netapi integration tests) would match it as a dead minion. Remove it.
+    salt_master.salt_key_cli().run("-d", factory.id, "-y")
 
 
 @pytest.mark.skipif(shutil.which("sudo") is None, reason="sudo is not available")

tests/pytests/integration/minion/test_startup_states.py

@@ -29,6 +29,10 @@ def salt_minion_startup_states_empty_string(salt_master, salt_minion_id):
     )
     with factory.started():
         yield factory
+    # The minion process is stopped at this point, but its accepted key stays
+    # on the shared session master, where later tests that target '*' (the
+    # netapi integration tests) would match it as a dead minion. Remove it.
+    salt_master.salt_key_cli().run("-d", factory.id, "-y")
 
 
 @pytest.fixture
@@ -43,6 +47,10 @@ def salt_minion_startup_states_highstate(salt_master, salt_minion_id):
     )
     with factory.started():
         yield factory
+    # The minion process is stopped at this point, but its accepted key stays
+    # on the shared session master, where later tests that target '*' (the
+    # netapi integration tests) would match it as a dead minion. Remove it.
+    salt_master.salt_key_cli().run("-d", factory.id, "-y")
 
 
 @pytest.fixture
@@ -58,6 +66,10 @@ def salt_minion_startup_states_sls(salt_master, salt_minion_id):
     )
     with factory.started():
         yield factory
+    # The minion process is stopped at this point, but its accepted key stays
+    # on the shared session master, where later tests that target '*' (the
+    # netapi integration tests) would match it as a dead minion. Remove it.
+    salt_master.salt_key_cli().run("-d", factory.id, "-y")
 
 
 @pytest.fixture
@@ -73,6 +85,10 @@ def salt_minion_startup_states_top(salt_master, salt_minion_id):
     )
     with factory.started():
         yield factory
+    # The minion process is stopped at this point, but its accepted key stays
+    # on the shared session master, where later tests that target '*' (the
+    # netapi integration tests) would match it as a dead minion. Remove it.
+    salt_master.salt_key_cli().run("-d", factory.id, "-y")
 
 
 def test_startup_states_empty_string(

tests/pytests/integration/ssh/conftest.py

@@ -66,7 +66,7 @@ def _reap_stray_processes():
 
 
 @pytest.fixture(scope="module")
-def state_tree(base_env_state_tree_root_dir):
+def state_tree(base_env_state_tree_root_dir, salt_ssh_cli):
     # Remove unused import from top file to avoid salt-ssh file sync issues
     # Note: top file references "basic" but we create "test.sls" - this appears
     # intentional as tests run state.sls directly and don't use the top file
@@ -96,6 +96,12 @@ def state_tree(base_env_state_tree_root_dir):
         "test.sls", state_file, base_env_state_tree_root_dir
     )
     with top_tempfile, map_tempfile, state_tempfile:
+        # slsutil.renderer over salt-ssh fetches the requested file but does
+        # not ship its jinja-imported files (map.jinja) to the target; only a
+        # state run syncs the full state tree to the target's file cache.
+        # Prime that cache once so the renderer tests are deterministic
+        # instead of depending on an earlier state test having warmed it.
+        salt_ssh_cli.run("state.apply", "test", test=True)
         yield
 
 

tests/pytests/unit/utils/event/test_event.py

@@ -1,4 +1,5 @@
 import hashlib
+import logging
 import os
 import stat
 import time
@@ -340,3 +341,71 @@ def test_master_pub_permissions(sock_dir):
         assert bool(os.lstat(p).st_mode & stat.S_IRUSR)
         assert not bool(os.lstat(p).st_mode & stat.S_IRGRP)
         assert not bool(os.lstat(p).st_mode & stat.S_IROTH)
+
+
+@pytest.fixture
+def ret_load_event(sock_dir):
+    with salt.utils.event.SaltEvent(
+        "master", str(sock_dir), opts={"transport": "zeromq"}, listen=False
+    ) as event:
+        with patch.object(event, "fire_event") as fire_event:
+            yield event, fire_event
+
+
+def test_fire_ret_load_list_return_skips_quietly_69730(ret_load_event, caplog):
+    """
+    A failing state compilation returns a list of error strings rather than
+    a mapping of per-state results. fire_ret_load used to hand that list to
+    _fire_ret_load_specific_fun, which crashed on ret.items() and logged
+    "Event iteration failed with exception: 'list' object has no attribute
+    'items'" at ERROR for every failed compile. There are no state tags in
+    such a return, so it must be skipped without logging an error and
+    without firing sub events.
+    """
+    event, fire_event = ret_load_event
+    # The exact shape the master receives for a failed state.apply compile:
+    # fun in SUB_EVENT, a non-zero retcode, and a list-of-errors return.
+    load = {
+        "id": "minion",
+        "jid": "20260706000000000000",
+        "fun": "state.sls",
+        "retcode": 1,
+        "return": ["Rendering SLS 'base:broken' failed: Jinja error"],
+    }
+    with caplog.at_level(logging.ERROR, logger="salt.utils.event"):
+        event.fire_ret_load(load)
+    assert "Event iteration failed" not in caplog.text
+    fire_event.assert_not_called()
+
+
+def test_fire_ret_load_dict_return_still_fires_sub_events_69730(ret_load_event, caplog):
+    """
+    Guard against overcorrection: a dict-shaped failing state return (the
+    normal case) must keep firing the per-tag failure events exactly as
+    before the non-dict guard was added. This passes with and without the
+    fix.
+    """
+    event, fire_event = ret_load_event
+    tag = "file_|-broken_|-/etc/broken_|-managed"
+    load = {
+        "id": "minion",
+        "jid": "20260706000000000000",
+        "fun": "state.sls",
+        "retcode": 2,
+        "return": {tag: {"result": False, "comment": "no such file"}},
+    }
+    with caplog.at_level(logging.ERROR, logger="salt.utils.event"):
+        event.fire_ret_load(load)
+    assert "Event iteration failed" not in caplog.text
+    assert fire_event.call_count == 2
+    # old-style duplicate event: <state>.<func> tag
+    first_data, first_tag = fire_event.call_args_list[0][0]
+    assert first_tag == "file.managed"
+    assert first_data["retcode"] == 2
+    # namespaced job sub event, enriched with job metadata
+    second_data, second_tag = fire_event.call_args_list[1][0]
+    assert second_tag == "salt/job/20260706000000000000/sub/minion/error/state.sls"
+    assert second_data["jid"] == "20260706000000000000"
+    assert second_data["id"] == "minion"
+    assert second_data["success"] is False
+    assert second_data["fun"] == "state.sls"