From b830c972e8213fdfed320c73ff65b36793b805bb Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sun, 5 Jul 2026 21:23:19 -0400 Subject: [PATCH 1/5] Deduplicate HTTP route index entries across netapi doc pages The rest_cherrypy, rest_tornado and rest_wsgi doc pages document overlapping HTTP routes, so each shared route registered multiple httpdomain index entries. sphinxcontrib-httpdomain only detects the duplicates in merge_domaindata, which runs when sphinx -j parallel reader chunks are merged, so the -W builds in tools docs (Prepare Release, Documentation) fail intermittently depending on where the chunk boundary lands: WARNING: duplicate HTTP post method definition / in doc/ref/netapi/all/salt.netapi.rest_tornado.rst, other instance is in doc/ref/netapi/all/salt.netapi.rest_wsgi.rst When it hits, the release patch artifact is never produced and every downstream build job in the run fails with 'Artifact not found'. Mark the tornado and wsgi copies of the shared routes with :noindex: so each route is registered exactly once, by the canonical rest_cherrypy reference. noindex'd directives never enter the domain data, so the merge collision is impossible under any chunking. Page content is unchanged and no :http: cross-references exist that could be affected. This also makes the HTTP routing index deterministic; it previously pointed at whichever page the readers processed last. Fixes #69724 --- changelog/69724.fixed.md | 1 + salt/netapi/rest_tornado/saltnado.py | 10 ++++++++++ salt/netapi/rest_wsgi.py | 1 + 3 files changed, 12 insertions(+) create mode 100644 changelog/69724.fixed.md diff --git a/changelog/69724.fixed.md b/changelog/69724.fixed.md new file mode 100644 index 000000000000..ea56ced3d3a2 --- /dev/null +++ b/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. diff --git a/salt/netapi/rest_tornado/saltnado.py b/salt/netapi/rest_tornado/saltnado.py index 99971b778b9d..079662ead805 100644 --- a/salt/netapi/rest_tornado/saltnado.py +++ b/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 ` 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): ` .. 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| diff --git a/salt/netapi/rest_wsgi.py b/salt/netapi/rest_wsgi.py index 50dfabe23c1e..1f448be4447d 100644 --- a/salt/netapi/rest_wsgi.py +++ b/salt/netapi/rest_wsgi.py @@ -65,6 +65,7 @@ ============== .. http:post:: / + :noindex: **Example request** for a basic ``test.ping``:: From f10d39dd69f677bbadcb33d6add8f3109c09d142 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sun, 5 Jul 2026 22:51:38 -0400 Subject: [PATCH 2/5] Keep HTML anchors on noindex'd httpdomain directives sphinxcontrib-httpdomain's add_target_and_index always appends the signature anchor and only gates the global route registration behind :noindex:, but Sphinx's ObjectDescription.run skips the whole method when noindex is set, so the previous commit's dedup also dropped the per-page anchors and permalinks from the rest_tornado and rest_wsgi endpoint signatures. Anchors are per-document HTML ids and cannot collide across pages; only the global registration can produce the parallel-merge duplicate warnings. Add a small extension that hides the noindex option from Sphinx's outer gate and re-presents it to httpdomain's inner gate, restoring the anchors and permalinks (existing deep links into those pages keep working) while the routes stay out of the domain data, so the duplicate-route collision remains impossible under any chunking. --- doc/_ext/salthttpanchors.py | 45 +++++++++++++++++++++++++++++++++++++ doc/conf.py | 1 + 2 files changed, 46 insertions(+) create mode 100644 doc/_ext/salthttpanchors.py diff --git a/doc/_ext/salthttpanchors.py b/doc/_ext/salthttpanchors.py new file mode 100644 index 000000000000..5822384a2a43 --- /dev/null +++ b/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 ``#--`` 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} diff --git a/doc/conf.py b/doc/conf.py index 64d38b0e7106..0450729512cc 100644 --- a/doc/conf.py +++ b/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 From 954ddaa976a5644807e51c67da3f3fb06877e706 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sun, 5 Jul 2026 22:32:09 -0400 Subject: [PATCH 3/5] Delete leaked minion keys at teardown in integration test fixtures The startup_states and salt_call ownership test modules start extra minions against the shared session master via 'with factory.started(): yield'. That stops the minion process at teardown but leaves the accepted key on the master, so later tests in the same session that target '*' (the netapi integration tests) key-match minions that no longer exist. The result is the Rocky Linux 9 integration tcp/zeromq pair failing most 3006.x PR runs: AssertionError: assert ['minion-X', 'minion-X-empty-string', 'minion-X-highstate', 'minion-X-sls', 'minion-X-top', 'non-root-minion-Y', 'sub-minion-Z'] == ['minion-X', 'sub-minion-Z'] plus 30 second TimeoutErrors waiting for returns from the dead minions. Nightly runs do not hit it because test sharding separates these modules from the netapi tests. Remove each extra minion's key from the master once the minion is stopped, restoring the isolation the netapi assertions rely on. Fixes #69728 --- changelog/69728.fixed.md | 1 + .../integration/cli/test_salt_call_ownership.py | 4 ++++ .../integration/minion/test_startup_states.py | 16 ++++++++++++++++ 3 files changed, 21 insertions(+) create mode 100644 changelog/69728.fixed.md diff --git a/changelog/69728.fixed.md b/changelog/69728.fixed.md new file mode 100644 index 000000000000..ca9a2e1b7240 --- /dev/null +++ b/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. diff --git a/tests/pytests/integration/cli/test_salt_call_ownership.py b/tests/pytests/integration/cli/test_salt_call_ownership.py index 175931da6079..8fe529328b80 100644 --- a/tests/pytests/integration/cli/test_salt_call_ownership.py +++ b/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") diff --git a/tests/pytests/integration/minion/test_startup_states.py b/tests/pytests/integration/minion/test_startup_states.py index d8b891b35c42..f76760b0ba21 100644 --- a/tests/pytests/integration/minion/test_startup_states.py +++ b/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( From 37e103940f984a7c8969c260aa367d9b1cdab2d6 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Mon, 6 Jul 2026 01:34:30 -0400 Subject: [PATCH 4/5] Skip sub events for non-dict job returns in the event tagger A failing state compilation returns a list of error strings (or a plain string from some renderers) rather than a mapping of per-state results. _fire_ret_load_specific_fun assumed a dict and crashed on ret.items(), logging 'Event iteration failed with exception: list object has no attribute items' at ERROR for every failed compile. The integration tests that assert that message never appears (tests/pytests/integration/states/test_state_test.py) fail whenever they share a run with a failing compile, which is part of the Rocky Linux 9 integration job noise on 3006.x. There are no state tags in a non-dict return, so skip it at debug level instead. Dict-shaped returns keep firing the per-tag failure events unchanged, covered by a new regression pair: the list-return test fails on the previous code via the logged error, and the dict-return test pins the two sub events (old-style dup tag and the namespaced job error tag) with their enriched payload. Fixes #69730 Refs #69728 --- changelog/69730.fixed.md | 1 + salt/utils/event.py | 12 ++++ tests/pytests/unit/utils/event/test_event.py | 69 ++++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 changelog/69730.fixed.md diff --git a/changelog/69730.fixed.md b/changelog/69730.fixed.md new file mode 100644 index 000000000000..f07f2f6bc20a --- /dev/null +++ b/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. diff --git a/salt/utils/event.py b/salt/utils/event.py index 6cfecd9e4e84..93a3d0518ea0 100644 --- a/salt/utils/event.py +++ b/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 diff --git a/tests/pytests/unit/utils/event/test_event.py b/tests/pytests/unit/utils/event/test_event.py index f4b6c1599966..3b3c2944bde6 100644 --- a/tests/pytests/unit/utils/event/test_event.py +++ b/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: . 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" From 392915d22c3cccd26f2f560dca02158036e4ac49 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Tue, 7 Jul 2026 01:35:20 -0400 Subject: [PATCH 5/5] Prime the salt-ssh file cache for the renderer tests test_slsutil.py::test_renderer_file runs slsutil.renderer on a file that does {% from "map.jinja" import abc %}. Over salt-ssh, slsutil.renderer fetches the requested file but does not ship its jinja-imported files to the target; only a state run syncs the full state tree to the target's file cache. So the render fails with 'Jinja error: map.jinja / TemplateNotFound' unless an earlier test in the same session already ran a state command that warmed the shared salt-ssh target cache -- flaky by test ordering in CI, deterministic when run in isolation. Prime the cache once in the state_tree fixture with a test-mode state apply, so the renderer tests do not depend on another test having warmed it. Verified against a local 3006.x reproduction: the test failed 3/3 in isolation before and passes 3/3 after, with the rest of the module unaffected. Fixes #69738 --- changelog/69738.fixed.md | 1 + tests/pytests/integration/ssh/conftest.py | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 changelog/69738.fixed.md diff --git a/changelog/69738.fixed.md b/changelog/69738.fixed.md new file mode 100644 index 000000000000..17df8786228d --- /dev/null +++ b/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. diff --git a/tests/pytests/integration/ssh/conftest.py b/tests/pytests/integration/ssh/conftest.py index c451a4850e58..b123d9de473c 100644 --- a/tests/pytests/integration/ssh/conftest.py +++ b/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