fix: descriptor leaking in salt.utils.http.query

changelog/68456.fixed.md

@@ -0,0 +1 @@
+Fixed descriptor leaking in salt.utils.http.query

salt/utils/http.py

@@ -615,12 +615,15 @@ def query(
         req_kwargs = salt.utils.data.decode(req_kwargs, to_str=True)
 
         try:
-            download_client = SyncWrapper(
+            download_client_kwargs = (
+                {"max_body_size": max_body} if supports_max_body_size else {}
+            )
+            with SyncWrapper(
                 AsyncHTTPClient,
-                kwargs={"max_body_size": max_body} if supports_max_body_size else {},
+                kwargs=download_client_kwargs,
                 async_methods=["fetch"],
-            )
-            result = download_client.fetch(url_full, **req_kwargs)
+            ) as download_client:
+                result = download_client.fetch(url_full, **req_kwargs)
         except tornado.httpclient.HTTPError as exc:
             ret["status"] = exc.code
             ret["error"] = str(exc)

tests/pytests/unit/utils/test_http.py

@@ -3,13 +3,70 @@
 
 import pytest
 import requests
+import tornado.httpclient
 from pytestshellutils.utils import ports
 from werkzeug.wrappers import Response  # pylint: disable=3rd-party-module-not-gated
 
 import salt.utils.http as http
 from tests.support.mock import MagicMock, patch
 
 
+class _FakeFetchResponse:
+    """Simple object mimicking the bits of tornado's HTTPResponse we rely on."""
+
+    def __init__(self, code=200, body=b"payload", headers=None):
+        self.code = code
+        self.body = body
+        self.headers = headers or {"Content-Type": "text/plain; charset=utf-8"}
+
+
+@pytest.fixture
+def syncwrapper_stub(monkeypatch):
+    """Patch ``salt.utils.http.SyncWrapper`` with a controllable test double."""
+
+    class SyncWrapperStub:
+        fetch_return = _FakeFetchResponse()
+        fetch_side_effect = None
+        enter_calls = 0
+        close_calls = 0
+        fetch_calls = 0
+
+        def __init__(self, *args, **kwargs):
+            # Mirror SyncWrapper signature but we only need to track usage.
+            pass
+
+        def __enter__(self):
+            SyncWrapperStub.enter_calls += 1
+            return self
+
+        def __exit__(self, exc_type, exc, tb):
+            self.close()
+            # Propagate exceptions so http.query can handle them
+            return False
+
+        def close(self):
+            SyncWrapperStub.close_calls += 1
+
+        def fetch(self, *args, **kwargs):
+            SyncWrapperStub.fetch_calls += 1
+            if SyncWrapperStub.fetch_side_effect is not None:
+                raise SyncWrapperStub.fetch_side_effect
+            return SyncWrapperStub.fetch_return
+
+        @classmethod
+        def reset_counters(cls, *, clear_fetch=True):
+            cls.enter_calls = 0
+            cls.close_calls = 0
+            cls.fetch_calls = 0
+            if clear_fetch:
+                cls.fetch_side_effect = None
+                cls.fetch_return = _FakeFetchResponse()
+
+    monkeypatch.setattr(http, "SyncWrapper", SyncWrapperStub)
+    SyncWrapperStub.reset_counters()
+    return SyncWrapperStub
+
+
 def test_requests_session_verify_ssl_false(ssl_webserver, integration_files_dir):
     """
     test salt.utils.http.session when using verify_ssl
@@ -183,6 +240,9 @@ def test_query_tornado_httperror_no_response():
 
     mock_client = MagicMock()
     mock_client.fetch.side_effect = http_error
+    # http.query() uses SyncWrapper as a context manager; ensure
+    # __enter__() returns the mock_client itself.
+    mock_client.__enter__.return_value = mock_client
 
     # http.query() wraps tornado's AsyncHTTPClient in a SyncWrapper before
     # calling .fetch(); patch the wrapper so .fetch() raises the HTTPError.
@@ -195,6 +255,40 @@ def test_query_tornado_httperror_no_response():
     assert "body" not in ret
 
 
+def test_query_tornado_closes_syncwrapper_on_success(syncwrapper_stub):
+    syncwrapper_stub.reset_counters()
+    syncwrapper_stub.fetch_return = _FakeFetchResponse(body=b"test-body")
+
+    ret = http.query("http://example.com", backend="tornado", status=True)
+
+    assert syncwrapper_stub.enter_calls == 1
+    assert syncwrapper_stub.close_calls == 1
+    assert syncwrapper_stub.fetch_calls == 1
+    assert ret["body"] == "test-body"
+    assert ret["status"] == 200
+
+
+def test_query_tornado_closes_syncwrapper_on_http_error(syncwrapper_stub):
+    syncwrapper_stub.reset_counters()
+    response = MagicMock(body=b"", headers={"Content-Type": "text/plain"})
+    syncwrapper_stub.fetch_side_effect = tornado.httpclient.HTTPError(
+        599, "Unit test failure", response=response
+    )
+
+    ret = http.query(
+        "http://example.com",
+        backend="tornado",
+        status=True,
+        raise_error=True,
+    )
+
+    assert syncwrapper_stub.enter_calls == 1
+    assert syncwrapper_stub.close_calls == 1
+    assert syncwrapper_stub.fetch_calls == 1
+    assert ret["status"] == 599
+    assert "Unit test failure" in ret["error"]
+
+
 def test_parse_cookie_header():
     header = "; ".join(
         [