From 4bf18bd34880f7fd55046b7f79d95cbce500f313 Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:15:04 -0700 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=90=9B=20fix(seed):=20keep=20partial?= =?UTF-8?q?=20wheel=20images=20out=20of=20the=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The app-data seeder skips building a wheel image whenever the image folder exists and is not empty, but the build extracted straight into that folder and left whatever it had written when a step failed. A full disk, an antivirus lock or a killed process could leave a half-built image, and so does Windows without long path support when the app-data path falls in the narrow band where only the long PEP 639 license paths at the end of the pip wheel are over the limit. The first run fails loudly. Every later run with the same app-data then reuses the partial image, prints "added seed packages" and exits 0, and the pip it seeds has no METADATA, RECORD or INSTALLER, so pip list shows "pip None" and pip cannot upgrade or reinstall itself. The image is now built in a staging folder next to the installer folder and moved into place with os.replace once the RECORD fix-up and, for symlink images, the read-only marking are done. A failed build deletes the staging folder through the path the build used, which is the 8.3 short path when Windows had to switch to it, and the next build deletes one that a killed process left behind. The staging path is exactly as long as the image path, so the Windows path length handling makes the same choices as before, and when it switches to the short path the install reads through the shortened parent path. --- .../embed/via_app_data/pip_install/base.py | 30 +++++- .../embed/via_app_data/pip_install/symlink.py | 4 +- .../embed/test_bootstrap_link_via_app_data.py | 100 ++++++++++++++++++ 3 files changed, 130 insertions(+), 4 deletions(-) diff --git a/src/virtualenv/seed/embed/via_app_data/pip_install/base.py b/src/virtualenv/seed/embed/via_app_data/pip_install/base.py index 2fd3ac260..95e228fee 100644 --- a/src/virtualenv/seed/embed/via_app_data/pip_install/base.py +++ b/src/virtualenv/seed/embed/via_app_data/pip_install/base.py @@ -9,6 +9,7 @@ import zipfile from abc import ABC, abstractmethod from configparser import ConfigParser +from contextlib import suppress from itertools import chain from pathlib import Path from tempfile import mkdtemp @@ -70,14 +71,39 @@ def install(self, version_info: tuple[int, ...]) -> None: LOGGER.debug("generated console scripts %s", " ".join(i.name for i in consoles)) def build_image(self) -> None: - """Extract the seed wheel into the image directory and fix up its RECORD file. + """Extract the seed wheel into a staging directory, fix up its RECORD file, then move it into place. Each archive entry is validated before extraction so a tampered wheel cannot escape the image directory via an - absolute path or ``..`` traversal. + absolute path or ``..`` traversal. Later runs reuse any image directory they find, so it only appears once the + image is complete: a build that fails part way, for example on a full disk or a path that is too long, removes + its staging directory, and the next build removes one that a killed process left behind. :raises RuntimeError: if the wheel contains an entry that would land outside the image directory. """ + image_dir: Final = self._image_dir + # beside the installer folder the staging path is exactly as long as the image path, so the Windows path length + # limits hit the build the same way they would hit the image + staging: Final = image_dir.parent.with_name(f"{image_dir.parent.name}-{image_dir.name}") + if staging.exists(): + safe_delete(staging) + image_dir.parent.mkdir(parents=True, exist_ok=True) + staging.mkdir() + self._image_dir = staging + try: + self._populate_image() + # keep the short path Windows may have switched the staging directory to, the install reads through it + built: Final = self._image_dir.parent / image_dir.parent.name / image_dir.name + with suppress(FileNotFoundError): + built.rmdir() # an empty image directory counts as missing, but blocks the rename on Windows + os.replace(self._image_dir, built) + except BaseException: + safe_delete(self._image_dir) # the staging directory, through the short path if Windows switched to it + raise + self._image_dir = built + self.__dist_info = None # it points into the staging directory + + def _populate_image(self) -> None: # 1. first extract the wheel LOGGER.debug("build install image for %s to %s", self._wheel.name, self._image_dir) with zipfile.ZipFile(str(self._wheel)) as zip_ref: diff --git a/src/virtualenv/seed/embed/via_app_data/pip_install/symlink.py b/src/virtualenv/seed/embed/via_app_data/pip_install/symlink.py index 1b367a856..ccdcb5190 100644 --- a/src/virtualenv/seed/embed/via_app_data/pip_install/symlink.py +++ b/src/virtualenv/seed/embed/via_app_data/pip_install/symlink.py @@ -57,8 +57,8 @@ def _fix_records(self, extra_record_data: set[Path]) -> None: extra_record_data_str = self._records_text(sorted(extra_record_data, key=str)) # ty: ignore[invalid-argument-type] (self._dist_info / "RECORD").write_text(extra_record_data_str, encoding="utf-8") # ty: ignore[unsupported-operator] - def build_image(self) -> None: - super().build_image() + def _populate_image(self) -> None: + super()._populate_image() # protect the image by making it read only set_tree(self._image_dir, S_IREAD | S_IRGRP | S_IROTH) diff --git a/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py b/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py index 8fafbf938..d7a02b6fb 100644 --- a/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py +++ b/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py @@ -2,6 +2,7 @@ import contextlib import csv +import errno import hashlib import logging import os @@ -539,6 +540,105 @@ def _run(name) -> None: return exceptions +def _fail_during_extraction(zip_ref: zipfile.ZipFile, target_dir: Path) -> None: + # stop short of the last entries, where the wheel keeps its dist-info files + for info in zip_ref.infolist()[:-5]: + zip_ref.extract(info, str(target_dir)) + raise OSError(errno.ENOSPC, "No space left on device") + + +def _fail_after_extraction(*_: object) -> None: + raise OSError(errno.ENOSPC, "No space left on device") + + +@pytest.mark.parametrize( + ("target", "failure", "linked"), + [ + pytest.param("base._safe_extract_zip", _fail_during_extraction, False, id="copy-extract"), + pytest.param("base.PipInstall._generate_new_files", _fail_after_extraction, False, id="copy-finish"), + pytest.param( + "base._safe_extract_zip", + _fail_during_extraction, + True, + id="symlink-extract", + marks=pytest.mark.skipif(not fs_supports_symlink(), reason="symlinks are unavailable"), + ), + pytest.param( + "base.PipInstall._generate_new_files", + _fail_after_extraction, + True, + id="symlink-finish", + marks=pytest.mark.skipif(not fs_supports_symlink(), reason="symlinks are unavailable"), + ), + pytest.param( + "symlink.set_tree", + _fail_after_extraction, + True, + id="symlink-read-only", + marks=pytest.mark.skipif(not fs_supports_symlink(), reason="symlinks are unavailable"), + ), + ], +) +def test_seed_image_build_failure_leaves_no_partial_image( + tmp_path: Path, mocker: MockerFixture, target: str, failure: Callable[..., None], linked: bool +) -> None: + cache: Final = tmp_path / "cache" + args: Final = [ + str(tmp_path / "env"), + "--app-data", + str(cache), + "--no-periodic-update", + "--no-setuptools", + *(["--symlink-app-data"] if linked else []), + ] + patched: Final = mocker.patch(f"virtualenv.seed.embed.via_app_data.pip_install.{target}", side_effect=failure) + with pytest.raises(RuntimeError, match="No space left on device"): + cli_run(args, setup_logging=False) + mocker.stop(patched) + installer: Final = "SymlinkPipInstall" if linked else "CopyPipInstall" + images: Final = next(cache.glob("wheel/*/image/1")) + left: Final = sorted(i.name for i in [*images.iterdir(), *(images / installer).iterdir()] if i.is_dir()) + purelib: Final = cli_run(args, setup_logging=False).creator.purelib + dist_info: Final = next(purelib.glob("pip-*.dist-info")) + assert ( + left, + (dist_info / "INSTALLER").is_file(), + (dist_info / "RECORD").is_file(), + (purelib / f"{dist_info.stem}.virtualenv").is_file(), + ) == ([installer], True, True, True) + + +@pytest.mark.parametrize( + ("leftover", "kept"), + [pytest.param("staging", False, id="killed-build"), pytest.param("image", True, id="empty-image")], +) +def test_seed_image_build_replaces_leftovers(tmp_path: Path, leftover: str, kept: bool) -> None: + cache: Final = tmp_path / "cache" + version: Final = PythonInfo.current_system().version_release_str + wheel: Final = Path(BUNDLE_SUPPORT[version]["pip"]).stem + installer: Final = cache / "wheel" / version / "image" / "1" / "CopyPipInstall" + path: Final = installer.with_name(f"{installer.name}-{wheel}") if leftover == "staging" else installer / wheel + path.mkdir(parents=True) + if leftover == "staging": + (path / "pip.py").write_text("", encoding="utf-8") + args: Final = [str(tmp_path / "env"), "--app-data", str(cache), "--no-periodic-update", "--no-setuptools"] + purelib: Final = cli_run([*args, "--pip", "embed"], setup_logging=False).creator.purelib + assert (path.exists(), (next(purelib.glob("pip-*.dist-info")) / "RECORD").is_file()) == (kept, True) + + +def test_seed_image_build_direct_call(tmp_path: Path) -> None: + session: Final = cli_run([str(tmp_path / "env"), "--no-seed"], setup_logging=False) + wheel: Final = BUNDLE_FOLDER / BUNDLE_SUPPORT[session.interpreter.version_release_str]["pip"] + image: Final = tmp_path / "image" / "CopyPipInstall" / wheel.stem + installer: Final = CopyPipInstall(wheel, session.creator, image) + installer.build_image() + assert ( + sorted(i.name for i in image.parent.parent.iterdir()), + (next(image.glob("*.dist-info")) / "INSTALLER").is_file(), + installer._dist_info.parent, # ruff:ignore[private-member-access] # the cache must not point into staging + ) == (["CopyPipInstall"], True, image) + + def _write_zip_with_entry(zip_path: Path, entry_name: str) -> None: with zipfile.ZipFile(str(zip_path), "w") as archive: archive.writestr(entry_name, "payload") From cc5eb272159aee27d06c6053a5cb17e17335303b Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:58:30 -0700 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=93=9D=20docs(changelog):=20add=20the?= =?UTF-8?q?=20news=20fragment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/changelog/+atomic-seed-image.bugfix.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 docs/changelog/+atomic-seed-image.bugfix.rst diff --git a/docs/changelog/+atomic-seed-image.bugfix.rst b/docs/changelog/+atomic-seed-image.bugfix.rst new file mode 100644 index 000000000..c2ad0f520 --- /dev/null +++ b/docs/changelog/+atomic-seed-image.bugfix.rst @@ -0,0 +1,3 @@ +Build the seed wheel image in a staging folder and move it into place only once it is complete, so an extraction that +fails part way, for example on a full disk, after a killed process or on a path over the Windows length limit, no +longer leaves a partial image that later runs seed as a pip without ``METADATA``, ``RECORD`` or ``INSTALLER``. From 142ac024f8d892aebd0f876d8d3301df3712e6ae Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 28 Oct 2026 04:00:16 +0000 Subject: [PATCH 3/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- docs/changelog/+atomic-seed-image.bugfix.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/changelog/+atomic-seed-image.bugfix.rst b/docs/changelog/+atomic-seed-image.bugfix.rst index c2ad0f520..ff8f4e125 100644 --- a/docs/changelog/+atomic-seed-image.bugfix.rst +++ b/docs/changelog/+atomic-seed-image.bugfix.rst @@ -1,3 +1,3 @@ Build the seed wheel image in a staging folder and move it into place only once it is complete, so an extraction that -fails part way, for example on a full disk, after a killed process or on a path over the Windows length limit, no -longer leaves a partial image that later runs seed as a pip without ``METADATA``, ``RECORD`` or ``INSTALLER``. +fails part way, for example on a full disk, after a killed process or on a path over the Windows length limit, no longer +leaves a partial image that later runs seed as a pip without ``METADATA``, ``RECORD`` or ``INSTALLER``. From 7f19240047bc9828f0e12b1a50d86a3a27214c63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bern=C3=A1t=20G=C3=A1bor?= Date: Tue, 27 Oct 2026 21:39:51 -0700 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=A7=AA=20test(seed):=20drop=20private?= =?UTF-8?q?=20access=20in=20image=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The direct build test read the private dist-info cache under a lint suppression; it now installs the built image and checks the pip script. The leftover test takes its path and files from the parameters instead of branching, and the failure parameter is typed without an ellipsis by passing the disk-full error itself where no partial extraction runs. --- .../embed/test_bootstrap_link_via_app_data.py | 43 +++++++++++-------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py b/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py index d7a02b6fb..c612d212d 100644 --- a/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py +++ b/tests/unit/seed/embed/test_bootstrap_link_via_app_data.py @@ -540,22 +540,21 @@ def _run(name) -> None: return exceptions +_DISK_FULL: Final = OSError(errno.ENOSPC, "No space left on device") + + def _fail_during_extraction(zip_ref: zipfile.ZipFile, target_dir: Path) -> None: # stop short of the last entries, where the wheel keeps its dist-info files for info in zip_ref.infolist()[:-5]: zip_ref.extract(info, str(target_dir)) - raise OSError(errno.ENOSPC, "No space left on device") - - -def _fail_after_extraction(*_: object) -> None: - raise OSError(errno.ENOSPC, "No space left on device") + raise _DISK_FULL @pytest.mark.parametrize( ("target", "failure", "linked"), [ pytest.param("base._safe_extract_zip", _fail_during_extraction, False, id="copy-extract"), - pytest.param("base.PipInstall._generate_new_files", _fail_after_extraction, False, id="copy-finish"), + pytest.param("base.PipInstall._generate_new_files", _DISK_FULL, False, id="copy-finish"), pytest.param( "base._safe_extract_zip", _fail_during_extraction, @@ -565,14 +564,14 @@ def _fail_after_extraction(*_: object) -> None: ), pytest.param( "base.PipInstall._generate_new_files", - _fail_after_extraction, + _DISK_FULL, True, id="symlink-finish", marks=pytest.mark.skipif(not fs_supports_symlink(), reason="symlinks are unavailable"), ), pytest.param( "symlink.set_tree", - _fail_after_extraction, + _DISK_FULL, True, id="symlink-read-only", marks=pytest.mark.skipif(not fs_supports_symlink(), reason="symlinks are unavailable"), @@ -580,7 +579,11 @@ def _fail_after_extraction(*_: object) -> None: ], ) def test_seed_image_build_failure_leaves_no_partial_image( - tmp_path: Path, mocker: MockerFixture, target: str, failure: Callable[..., None], linked: bool + tmp_path: Path, + mocker: MockerFixture, + target: str, + failure: Callable[[zipfile.ZipFile, Path], None] | OSError, + linked: bool, ) -> None: cache: Final = tmp_path / "cache" args: Final = [ @@ -609,18 +612,19 @@ def test_seed_image_build_failure_leaves_no_partial_image( @pytest.mark.parametrize( - ("leftover", "kept"), - [pytest.param("staging", False, id="killed-build"), pytest.param("image", True, id="empty-image")], + ("leftover", "files", "kept"), + [ + pytest.param("CopyPipInstall-{}", ("pip.py",), False, id="killed-build"), + pytest.param("CopyPipInstall/{}", (), True, id="empty-image"), + ], ) -def test_seed_image_build_replaces_leftovers(tmp_path: Path, leftover: str, kept: bool) -> None: +def test_seed_image_build_replaces_leftovers(tmp_path: Path, leftover: str, files: tuple[str, ...], kept: bool) -> None: cache: Final = tmp_path / "cache" version: Final = PythonInfo.current_system().version_release_str - wheel: Final = Path(BUNDLE_SUPPORT[version]["pip"]).stem - installer: Final = cache / "wheel" / version / "image" / "1" / "CopyPipInstall" - path: Final = installer.with_name(f"{installer.name}-{wheel}") if leftover == "staging" else installer / wheel + path: Final = cache / "wheel" / version / "image" / "1" / leftover.format(Path(BUNDLE_SUPPORT[version]["pip"]).stem) path.mkdir(parents=True) - if leftover == "staging": - (path / "pip.py").write_text("", encoding="utf-8") + for name in files: + (path / name).write_text("", encoding="utf-8") args: Final = [str(tmp_path / "env"), "--app-data", str(cache), "--no-periodic-update", "--no-setuptools"] purelib: Final = cli_run([*args, "--pip", "embed"], setup_logging=False).creator.purelib assert (path.exists(), (next(purelib.glob("pip-*.dist-info")) / "RECORD").is_file()) == (kept, True) @@ -632,11 +636,12 @@ def test_seed_image_build_direct_call(tmp_path: Path) -> None: image: Final = tmp_path / "image" / "CopyPipInstall" / wheel.stem installer: Final = CopyPipInstall(wheel, session.creator, image) installer.build_image() + installer.install(session.interpreter.version_info) assert ( sorted(i.name for i in image.parent.parent.iterdir()), (next(image.glob("*.dist-info")) / "INSTALLER").is_file(), - installer._dist_info.parent, # ruff:ignore[private-member-access] # the cache must not point into staging - ) == (["CopyPipInstall"], True, image) + session.creator.script("pip").exists(), + ) == (["CopyPipInstall"], True, True) def _write_zip_with_entry(zip_path: Path, entry_name: str) -> None: