From 19aabff97eec20b5588ac08dc62fdc4e650cc7ef Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Thu, 8 Oct 2026 23:12:28 -0700 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20fix(cache):=20drop=20a=20cac?= =?UTF-8?q?he=20entry=20of=20the=20wrong=20shape=20instead=20of=20crashing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DiskCache.read() already drops a py_info cache file that fails to parse, but valid JSON of the wrong shape got through. A list, string, number or boolean at the top level reached _read_cache_entry, whose data.get() raised AttributeError. A content mapping with no system_executable did the same, and a non-path one (an int, a list) made Path() raise TypeError. Nothing above caught either, so a single damaged file under the cache failed every interpreter lookup that hit it. virtualenv, for one, stops creating environments until someone deletes the file. Treat any of these the way a file that fails to parse is treated: remove it and query the interpreter again. That matches what _load_cached_py_info already did for a bad version_info. --- src/python_discovery/_cached_py_info.py | 12 +++++--- tests/test_cached_py_info.py | 41 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/python_discovery/_cached_py_info.py b/src/python_discovery/_cached_py_info.py index e28aa0c..d9311ca 100644 --- a/src/python_discovery/_cached_py_info.py +++ b/src/python_discovery/_cached_py_info.py @@ -136,7 +136,8 @@ def _read_cache_entry( ) -> PythonInfo | _UnsupportedInterpreterError | None: if not py_info_store.exists() or (data := py_info_store.read()) is None: return None - if all(data.get(key) == value for key, value in entry_meta.items()): + # Valid JSON that is not an object is as unusable as a file that fails to parse; drop it the same way. + if isinstance(data, dict) and all(data.get(key) == value for key, value in entry_meta.items()): if isinstance(unsupported := data.get("unsupported"), str): return _UnsupportedInterpreterError(unsupported) if isinstance(content := data.get("content"), dict): @@ -152,10 +153,11 @@ def _load_cached_py_info( ) -> PythonInfo | None: try: py_info = cls.from_dict(content.copy()) - except (KeyError, TypeError): - py_info_store.remove() - return None - if (sys_exe := py_info.system_executable) is not None and not Path(sys_exe).exists(): + # A missing or non-path system_executable fails here rather than at from_dict, which copies keys unchecked. + stale = (sys_exe := py_info.system_executable) is not None and not Path(sys_exe).exists() + except (AttributeError, KeyError, TypeError): + stale = True + if stale: py_info_store.remove() return None return py_info diff --git a/tests/test_cached_py_info.py b/tests/test_cached_py_info.py index 8325621..3f4c523 100644 --- a/tests/test_cached_py_info.py +++ b/tests/test_cached_py_info.py @@ -161,6 +161,25 @@ def test_load_cached_py_info_bad_data() -> None: store.remove.assert_called_once() +@pytest.mark.parametrize( + "system_executable", + [pytest.param(5, id="int"), pytest.param(["python"], id="list"), pytest.param({}, id="dict")], +) +def test_load_cached_py_info_system_exe_wrong_type(system_executable: object) -> None: + store = MagicMock() + content = {**PythonInfo().to_dict(), "system_executable": system_executable} + assert _load_cached_py_info(PythonInfo, store, content) is None + store.remove.assert_called_once() + + +def test_load_cached_py_info_system_exe_absent() -> None: + store = MagicMock() + content = PythonInfo().to_dict() + del content["system_executable"] + assert _load_cached_py_info(PythonInfo, store, content) is None + store.remove.assert_called_once() + + def test_load_cached_py_info_system_exe_missing(mocker: MockerFixture) -> None: store = MagicMock() content = PythonInfo().to_dict() @@ -201,6 +220,28 @@ def test_get_via_file_cache_stale_hash(tmp_path: Path) -> None: assert isinstance(result2, PythonInfo) +@pytest.mark.parametrize( + "payload", + [ + pytest.param([], id="list"), + pytest.param("python", id="string"), + pytest.param(3, id="number"), + pytest.param(True, id="bool"), + ], +) +def test_get_via_file_cache_drops_non_object_entry(tmp_path: Path, payload: object) -> None: + cache = DiskCache(tmp_path) + path = Path(sys.executable) + env = dict(os.environ) + assert isinstance(_get_via_file_cache(PythonInfo, cache, path, sys.executable, env), PythonInfo) + + store = cache.py_info(path) + store.write(payload) # ty: ignore[invalid-argument-type] # the cache file holds whatever was last written there + + assert isinstance(_get_via_file_cache(PythonInfo, cache, path, sys.executable, env), PythonInfo) + assert isinstance(store.read(), dict) + + def test_get_via_file_cache_nonexistent_path(tmp_path: Path) -> None: cache = DiskCache(tmp_path) path = Path(tmp_path / "nonexistent") From 4d5a50290517a5970614ccc4bbf50ff2d66b1206 Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Thu, 8 Oct 2026 23:24:36 -0700 Subject: [PATCH 2/3] docs: add changelog fragment for 141 --- docs/changelog/141.bugfix.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 docs/changelog/141.bugfix.rst diff --git a/docs/changelog/141.bugfix.rst b/docs/changelog/141.bugfix.rst new file mode 100644 index 0000000..b4147e4 --- /dev/null +++ b/docs/changelog/141.bugfix.rst @@ -0,0 +1,3 @@ +Drop a py_info cache entry that holds valid JSON of the wrong shape, such as a top-level list or a missing or +non-path ``system_executable``, and query the interpreter again instead of raising ``AttributeError`` or ``TypeError`` +- by :user:`darrenhuai`. From 7118604f093f53a7c94f3080c20227efa081aa28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bern=C3=A1t=20G=C3=A1bor?= Date: Fri, 9 Oct 2026 17:27:17 -0700 Subject: [PATCH 3/3] fix(cache): reject unusable cached paths Cached directories and empty paths interrupt host resolution. Treat them and filesystem lookup errors as stale entries, then query the interpreter again. Verify cache recovery through the public API with real files. --- docs/changelog/141.bugfix.rst | 5 +- src/python_discovery/_cached_py_info.py | 6 +- tests/test_cache_recovery.py | 74 +++++++++++++++++++++++++ tests/test_cached_py_info.py | 41 -------------- 4 files changed, 78 insertions(+), 48 deletions(-) create mode 100644 tests/test_cache_recovery.py diff --git a/docs/changelog/141.bugfix.rst b/docs/changelog/141.bugfix.rst index b4147e4..a0b3f7d 100644 --- a/docs/changelog/141.bugfix.rst +++ b/docs/changelog/141.bugfix.rst @@ -1,3 +1,2 @@ -Drop a py_info cache entry that holds valid JSON of the wrong shape, such as a top-level list or a missing or -non-path ``system_executable``, and query the interpreter again instead of raising ``AttributeError`` or ``TypeError`` -- by :user:`darrenhuai`. +Discard malformed interpreter cache entries and query the interpreter again, including entries with missing, +non-string or unusable ``system_executable`` paths - by :user:`darrenhuai`. diff --git a/src/python_discovery/_cached_py_info.py b/src/python_discovery/_cached_py_info.py index d9311ca..f2f8266 100644 --- a/src/python_discovery/_cached_py_info.py +++ b/src/python_discovery/_cached_py_info.py @@ -136,7 +136,6 @@ def _read_cache_entry( ) -> PythonInfo | _UnsupportedInterpreterError | None: if not py_info_store.exists() or (data := py_info_store.read()) is None: return None - # Valid JSON that is not an object is as unusable as a file that fails to parse; drop it the same way. if isinstance(data, dict) and all(data.get(key) == value for key, value in entry_meta.items()): if isinstance(unsupported := data.get("unsupported"), str): return _UnsupportedInterpreterError(unsupported) @@ -153,9 +152,8 @@ def _load_cached_py_info( ) -> PythonInfo | None: try: py_info = cls.from_dict(content.copy()) - # A missing or non-path system_executable fails here rather than at from_dict, which copies keys unchecked. - stale = (sys_exe := py_info.system_executable) is not None and not Path(sys_exe).exists() - except (AttributeError, KeyError, TypeError): + stale = (sys_exe := py_info.system_executable) is not None and not Path(sys_exe).is_file() + except (AttributeError, KeyError, OSError, TypeError): stale = True if stale: py_info_store.remove() diff --git a/tests/test_cache_recovery.py b/tests/test_cache_recovery.py new file mode 100644 index 0000000..5d332d5 --- /dev/null +++ b/tests/test_cache_recovery.py @@ -0,0 +1,74 @@ +from __future__ import annotations + +import json +import sys +from typing import TYPE_CHECKING + +import pytest + +from python_discovery import DiskCache, PythonInfo + +if TYPE_CHECKING: + from pathlib import Path + + +@pytest.mark.parametrize( + "payload", + [ + pytest.param("[]", id="list"), + pytest.param('"python"', id="string"), + pytest.param("3", id="number"), + pytest.param("true", id="bool"), + pytest.param("null", id="null"), + ], +) +def test_from_exe_recovers_non_object_cache(cached_interpreter: tuple[DiskCache, Path, dict], payload: str) -> None: + cache, cache_file, expected = cached_interpreter + cache_file.write_text(payload, encoding="utf-8") + result = PythonInfo.from_exe(sys.executable, cache, ignore_cache=True, resolve_to_host=False) + assert result is not None + assert (json.loads(result.to_json()), json.loads(cache_file.read_text(encoding="utf-8"))) == ( + expected["content"], + expected, + ) + + +@pytest.mark.parametrize( + "payload", + [ + pytest.param(None, id="missing"), + pytest.param("5", id="int"), + pytest.param('["python"]', id="list"), + pytest.param("{}", id="dict"), + pytest.param("true", id="bool"), + pytest.param('""', id="empty"), + pytest.param('"."', id="directory"), + pytest.param('"missing-python"', id="nonexistent"), + pytest.param('"python\\u0000"', id="nul"), + pytest.param(json.dumps("a" * 1024), id="path-too-long"), + ], +) +def test_from_exe_recovers_invalid_system_executable( + cached_interpreter: tuple[DiskCache, Path, dict], payload: str | None +) -> None: + cache, cache_file, expected = cached_interpreter + data = json.loads(cache_file.read_text(encoding="utf-8")) + if payload is None: + del data["content"]["system_executable"] + else: + data["content"]["system_executable"] = json.loads(payload) + cache_file.write_text(json.dumps(data), encoding="utf-8") + result = PythonInfo.from_exe(sys.executable, cache, ignore_cache=True, resolve_to_host=False) + assert result is not None + assert (json.loads(result.to_json()), json.loads(cache_file.read_text(encoding="utf-8"))) == ( + expected["content"], + expected, + ) + + +@pytest.fixture +def cached_interpreter(tmp_path: Path) -> tuple[DiskCache, Path, dict]: + cache = DiskCache(tmp_path) + assert PythonInfo.from_exe(sys.executable, cache, ignore_cache=True, resolve_to_host=False) is not None + cache_file = next(tmp_path.rglob("*.json")) + return cache, cache_file, json.loads(cache_file.read_text(encoding="utf-8")) diff --git a/tests/test_cached_py_info.py b/tests/test_cached_py_info.py index 3f4c523..8325621 100644 --- a/tests/test_cached_py_info.py +++ b/tests/test_cached_py_info.py @@ -161,25 +161,6 @@ def test_load_cached_py_info_bad_data() -> None: store.remove.assert_called_once() -@pytest.mark.parametrize( - "system_executable", - [pytest.param(5, id="int"), pytest.param(["python"], id="list"), pytest.param({}, id="dict")], -) -def test_load_cached_py_info_system_exe_wrong_type(system_executable: object) -> None: - store = MagicMock() - content = {**PythonInfo().to_dict(), "system_executable": system_executable} - assert _load_cached_py_info(PythonInfo, store, content) is None - store.remove.assert_called_once() - - -def test_load_cached_py_info_system_exe_absent() -> None: - store = MagicMock() - content = PythonInfo().to_dict() - del content["system_executable"] - assert _load_cached_py_info(PythonInfo, store, content) is None - store.remove.assert_called_once() - - def test_load_cached_py_info_system_exe_missing(mocker: MockerFixture) -> None: store = MagicMock() content = PythonInfo().to_dict() @@ -220,28 +201,6 @@ def test_get_via_file_cache_stale_hash(tmp_path: Path) -> None: assert isinstance(result2, PythonInfo) -@pytest.mark.parametrize( - "payload", - [ - pytest.param([], id="list"), - pytest.param("python", id="string"), - pytest.param(3, id="number"), - pytest.param(True, id="bool"), - ], -) -def test_get_via_file_cache_drops_non_object_entry(tmp_path: Path, payload: object) -> None: - cache = DiskCache(tmp_path) - path = Path(sys.executable) - env = dict(os.environ) - assert isinstance(_get_via_file_cache(PythonInfo, cache, path, sys.executable, env), PythonInfo) - - store = cache.py_info(path) - store.write(payload) # ty: ignore[invalid-argument-type] # the cache file holds whatever was last written there - - assert isinstance(_get_via_file_cache(PythonInfo, cache, path, sys.executable, env), PythonInfo) - assert isinstance(store.read(), dict) - - def test_get_via_file_cache_nonexistent_path(tmp_path: Path) -> None: cache = DiskCache(tmp_path) path = Path(tmp_path / "nonexistent")