fix(query): validate PY_DISCOVERY_TIMEOUT

docs/changelog/143.bugfix.rst

@@ -0,0 +1,2 @@
+Use the default interpreter query timeout and warn for invalid ``PY_DISCOVERY_TIMEOUT`` values. Accept ``inf``
+or values above the subprocess wait limit to disable the timeout - by :user:`darrenhuai`.

docs/explanation.rst

@@ -275,7 +275,8 @@ If your system consistently hits timeouts, you can customize the timeout via the
 
 The timeout applies to each individual interpreter being queried. If you set a value that is too low,
 legitimate interpreters may be skipped; if too high, the discovery process may take longer to fail
-when encountering problematic interpreters.
+when encountering problematic interpreters. For invalid values, python-discovery logs a warning and uses
+the default of 15 seconds. Set ``inf`` to disable the timeout.
 
 Supported Python versions
 -------------------------

docs/reference/environment-variables.rst

@@ -36,3 +36,7 @@ Setting this variable extends the allowed time for each interpreter query.
 - Setting the value too low may skip legitimate interpreters
 - Setting it too high increases discovery time when encountering problematic interpreters
 - The value is read from the environment dict passed to :func:`~python_discovery.get_interpreter`
+- For invalid values, including empty strings, ``nan``, ``0`` and negative numbers, python-discovery logs a warning
+  and uses the default of 15 seconds
+- ``inf`` or a value above 2,147,483.647 seconds (about 24.9 days) disables the timeout. This limit applies on all
+  platforms because some subprocess waits accept a signed 32-bit count of milliseconds

src/python_discovery/_cached_py_info.py

@@ -41,6 +41,9 @@ class CacheEntryMeta(TypedDict):
 _CACHE[Path(sys.executable)] = PythonInfo()
 _LOGGER: Final[logging.Logger] = logging.getLogger(__name__)
 _PY_INFO_SCRIPT: Final[Path] = Path(__file__).resolve().parent / "_py_info_collect.py"
+_DEFAULT_QUERY_TIMEOUT: Final[float] = 15.0
+# poll(2) limits milliseconds to a signed C int.
+_MAX_QUERY_TIMEOUT: Final[float] = (2**31 - 1) / 1000
 
 
 class _UnsupportedInterpreterError(RuntimeError):
@@ -214,7 +217,7 @@ def _run_subprocess(
 ) -> tuple[Exception | None, PythonInfo | None]:
     start_cookie = gen_cookie()
     end_cookie = gen_cookie()
-    timeout = float(env.get("PY_DISCOVERY_TIMEOUT", "15"))
+    timeout = _query_timeout(env.get("PY_DISCOVERY_TIMEOUT"))
     with _resolve_py_info_script() as py_info_script:
         cmd = [exe, str(py_info_script), start_cookie, end_cookie]
         env = dict(env)
@@ -264,6 +267,22 @@ def _run_subprocess(
     return None, result
 
 
+@lru_cache(maxsize=128)
+def _query_timeout(raw: str | None) -> float | None:
+    if raw is None:
+        return _DEFAULT_QUERY_TIMEOUT
+    try:
+        timeout = float(raw)
+    except ValueError:
+        timeout = 0.0
+    if timeout > 0:
+        return timeout if timeout <= _MAX_QUERY_TIMEOUT else None
+    _LOGGER.warning(
+        "ignoring PY_DISCOVERY_TIMEOUT=%r, not a positive number of seconds; using %s", raw, _DEFAULT_QUERY_TIMEOUT
+    )
+    return _DEFAULT_QUERY_TIMEOUT
+
+
 def _query_failure(exe: str, out: str, err: str | None, code: int | None) -> RuntimeError:
     # the gate's exit code and stderr marker must both be present: either alone can collide, an errno of 79 from a
     # failed exec or a shim that echoes the marker phrase, but only the gate produces the combination

tests/test_cached_py_info.py

@@ -124,17 +124,6 @@ def test_run_subprocess_timeout(mocker: MockerFixture) -> None:
     assert mock_process.communicate.call_count == 2
 
 
-def test_run_subprocess_custom_timeout(mocker: MockerFixture) -> None:
-    mock_process = MagicMock()
-    mock_process.communicate.return_value = (json.dumps(PythonInfo().to_dict()), "")
-    mock_process.returncode = 0
-    mocker.patch("python_discovery._cached_py_info.Popen", return_value=mock_process)
-    env = dict(os.environ)
-    env["PY_DISCOVERY_TIMEOUT"] = "30"
-    _run_subprocess(PythonInfo, sys.executable, env)
-    mock_process.communicate.assert_called_once_with(timeout=30.0)
-
-
 def test_run_subprocess_nonzero_exit(mocker: MockerFixture) -> None:
     mock_process = MagicMock()
     mock_process.communicate.return_value = ("some output", "some error")

tests/test_query_timeout.py

@@ -0,0 +1,88 @@
+from __future__ import annotations
+
+import logging
+import os
+import sys
+from subprocess import Popen
+from typing import TYPE_CHECKING
+
+import pytest
+
+from python_discovery import PythonInfo
+
+if TYPE_CHECKING:
+    from pytest_mock import MockerFixture
+
+
+@pytest.mark.parametrize(
+    ("raw", "expected"),
+    [
+        pytest.param(None, 15.0, id="default"),
+        pytest.param("30", 30.0, id="seconds"),
+        pytest.param(" 0.5 ", 0.5, id="padded-fraction"),
+        pytest.param("inf", None, id="inf"),
+        pytest.param("1e30", None, id="beyond-platform-wait"),
+        pytest.param("2147483.647", 2147483.647, id="wait-limit"),
+        pytest.param("2147483.648", None, id="above-wait-limit"),
+    ],
+)
+def test_from_exe_query_timeout(
+    mocker: MockerFixture, caplog: pytest.LogCaptureFixture, raw: str | None, expected: float | None
+) -> None:
+    communicate = mocker.spy(Popen, "communicate")
+    env = {key: value for key, value in os.environ.items() if key != "PY_DISCOVERY_TIMEOUT"}
+    if raw is not None:
+        env["PY_DISCOVERY_TIMEOUT"] = raw
+    result = PythonInfo.from_exe(sys.executable, env=env, ignore_cache=True, resolve_to_host=False)
+    assert result is not None
+    assert result.version_info == sys.version_info
+    assert communicate.call_args_list[0].kwargs == {"timeout": expected}
+    assert not caplog.records
+
+
+@pytest.mark.parametrize(
+    "raw",
+    [
+        pytest.param("", id="empty"),
+        pytest.param("abc", id="words"),
+        pytest.param("0x10", id="hex"),
+        pytest.param("nan", id="nan"),
+        pytest.param("0", id="zero"),
+        pytest.param("-1", id="negative"),
+        pytest.param("-inf", id="negative-inf"),
+        pytest.param("1e-999", id="underflow"),
+    ],
+)
+def test_from_exe_invalid_timeout_warns_once(mocker: MockerFixture, caplog: pytest.LogCaptureFixture, raw: str) -> None:
+    communicate = mocker.spy(Popen, "communicate")
+    env = {**os.environ, "PY_DISCOVERY_TIMEOUT": raw}
+    for _ in range(2):
+        result = PythonInfo.from_exe(sys.executable, env=env, ignore_cache=True, resolve_to_host=False)
+        assert result is not None
+        assert result.version_info == sys.version_info
+    assert [call.kwargs for call in communicate.call_args_list] == [{"timeout": 15.0}] * 2
+    assert [(record.levelno, record.getMessage()) for record in caplog.records] == [
+        (logging.WARNING, f"ignoring PY_DISCOVERY_TIMEOUT={raw!r}, not a positive number of seconds; using 15.0")
+    ]
+
+
+def test_from_exe_timeout_warning_cache_is_bounded(caplog: pytest.LogCaptureFixture) -> None:
+    for index in range(129):
+        result = PythonInfo.from_exe(
+            sys.executable,
+            env={**os.environ, "PY_DISCOVERY_TIMEOUT": f"invalid-timeout-{index}"},
+            ignore_cache=True,
+            resolve_to_host=False,
+        )
+        assert result is not None
+        assert result.version_info == sys.version_info
+    caplog.clear()
+    for raw in ("invalid-timeout-128", "invalid-timeout-0"):
+        result = PythonInfo.from_exe(
+            sys.executable, env={**os.environ, "PY_DISCOVERY_TIMEOUT": raw}, ignore_cache=True, resolve_to_host=False
+        )
+        assert result is not None
+        assert result.version_info == sys.version_info
+    assert [record.getMessage() for record in caplog.records] == [
+        "ignoring PY_DISCOVERY_TIMEOUT='invalid-timeout-0', not a positive number of seconds; using 15.0"
+    ]