diff --git a/.github/workflows/python-tests.yml b/.github/workflows/python-tests.yml index 365fea73f1..3f90d2e677 100644 --- a/.github/workflows/python-tests.yml +++ b/.github/workflows/python-tests.yml @@ -825,6 +825,16 @@ jobs: - name: Check Windows collaboration lock sidecars run: python -m pytest -q --basetemp .t tests/test_collaboration_sidecar_scan.py + - name: Qualify the read-only fixture teardown on native Windows + # Windows refuses to unlink a read-only file and git marks every loose + # object read-only, so these suites fail on the pre-fix teardown with + # PermissionError [WinError 5] and cannot discriminate on POSIX. + run: >- + python -m pytest -q + tests/canary/test_smoke_fixture_cleanup.py + tests/extensions/test_process_runtime.py + tests/capabilities/test_issue_fix_temporary_workspace.py + - name: Qualify native Windows peer reads and long-path inputs run: >- python -m pytest -q diff --git a/examples/issue-fix-reviewer-recommendation-smoke.py b/examples/issue-fix-reviewer-recommendation-smoke.py index bd038ad09d..16f9d31722 100644 --- a/examples/issue-fix-reviewer-recommendation-smoke.py +++ b/examples/issue-fix-reviewer-recommendation-smoke.py @@ -7,7 +7,6 @@ import json import os import re -import shutil import subprocess import sys import tempfile @@ -26,6 +25,7 @@ ISSUE_FIX_REVIEWER_SOURCES_INPUT_SCHEMA_VERSION, build_issue_fix_reviewer_recommendation_packet, ) +from loopx.extensions.process_runtime import remove_owned_tree # noqa: E402 PRIVATE_PATTERNS = [ @@ -44,7 +44,7 @@ def temporary_git_repo() -> Iterator[Path]: finally: for attempt in range(10): try: - shutil.rmtree(path) + remove_owned_tree(path) break except FileNotFoundError: break diff --git a/examples/issue-fix-reviewer-request-smoke.py b/examples/issue-fix-reviewer-request-smoke.py index c5f004afa5..bbb75f5ef9 100644 --- a/examples/issue-fix-reviewer-request-smoke.py +++ b/examples/issue-fix-reviewer-request-smoke.py @@ -9,7 +9,6 @@ import json import os import re -import shutil import subprocess import sys import tempfile @@ -40,6 +39,7 @@ from loopx.capabilities.issue_fix.reviewer_notification_drain import ( # noqa: E402 drain_issue_fix_reviewer_notification_queue, ) +from loopx.extensions.process_runtime import remove_owned_tree # noqa: E402 from loopx.extensions.lark.reviewer_notification import ( # noqa: E402 lark_reviewer_notification_sink, ) @@ -2095,7 +2095,7 @@ def drain_metadata_loader( finally: for attempt in range(10): try: - shutil.rmtree(path) + remove_owned_tree(path) break except FileNotFoundError: break diff --git a/loopx/canary/runner.py b/loopx/canary/runner.py index 57bdc3fd0f..53ab3c4a62 100644 --- a/loopx/canary/runner.py +++ b/loopx/canary/runner.py @@ -3,7 +3,6 @@ import os import re import shlex -import shutil import subprocess import sys import tempfile @@ -12,7 +11,7 @@ from pathlib import Path from typing import Any, Callable -from ..extensions.process_runtime import terminate_process_tree +from ..extensions.process_runtime import remove_owned_tree, terminate_process_tree from .planner import ( REPO_ROOT, build_catalog_canary_plan, @@ -288,6 +287,17 @@ def _retire_fixture_runtimes(root: Path, *, timeout: float = 15) -> None: raise RuntimeError(f"managed runtime still exiting: {path.name}") +def _remove_fixture_root(root: Path) -> None: + """Stop every managed runtime in an owned smoke root, then remove the root. + + The root belongs to one check and holds host fixtures, git object stores + included, so removal tolerates the read-only attributes a host leaves behind. + A failure still raises, and the caller retains the tree for diagnosis. + """ + _retire_fixture_runtimes(root) + remove_owned_tree(root) + + def _run_check( check: dict[str, Any], *, @@ -347,8 +357,7 @@ def _run_check( ) finally: try: - _retire_fixture_runtimes(root) - shutil.rmtree(root) + _remove_fixture_root(root) except (OSError, RuntimeError, ValueError) as error: # Retain the owned namespace if shutdown cannot be proved. # Do not mask the check with unconditional second deletion. diff --git a/loopx/capabilities/issue_fix/acceptance_loop.py b/loopx/capabilities/issue_fix/acceptance_loop.py index 76e7acb876..3160877284 100644 --- a/loopx/capabilities/issue_fix/acceptance_loop.py +++ b/loopx/capabilities/issue_fix/acceptance_loop.py @@ -16,6 +16,7 @@ run_caller_validation as _run_caller_validation, require_validation_passed as _require_passed, ) +from ...extensions.process_runtime import remove_owned_tree ISSUE_FIX_ACCEPTANCE_LOOP_SCHEMA_VERSION = "issue_fix_acceptance_loop_v0" @@ -256,9 +257,16 @@ def add_lines(lines: list[str]) -> None: def _remove_temporary_git_workspace(workspace: Path) -> None: + """Drop this packet's own temporary repository, read-only objects included. + + Git marks every loose object read-only, which Windows refuses to unlink, so + the retry below only ever helped a transient lock; the host attribute is + cleared by the shared owner before each attempt. + """ + for attempt in range(5): try: - shutil.rmtree(workspace) + remove_owned_tree(workspace) return except OSError: if attempt == 4: diff --git a/loopx/extensions/process_runtime.py b/loopx/extensions/process_runtime.py index 1b1d6b2c52..4a5dcfd026 100644 --- a/loopx/extensions/process_runtime.py +++ b/loopx/extensions/process_runtime.py @@ -3,7 +3,9 @@ from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass import os +import shutil import signal +import stat import subprocess import sys import threading @@ -239,6 +241,98 @@ def cleanup() -> None: closed = True return cleanup +_WINDOWS_REPARSE_POINT = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0x400) + + +def _is_link_or_junction(target: str) -> bool: + """Whether ``target`` is a link this teardown must not traverse. + + ``os.path.islink`` covers POSIX symlinks and Windows file or directory + symlinks. A Windows junction is also a reparse point but is not reported as + a symlink, and ``os.walk`` still descends into it, so the reparse attribute + is checked explicitly; ``os.path.isjunction`` does not exist before 3.12. + """ + if os.path.islink(target): + return True + if os.name != "nt": + return False + try: + status = os.lstat(target) + except OSError: + return False + # Only a directory reparse point (junction or mount point) can redirect the + # walk; file reparse points such as cloud placeholders are not links, so + # their read-only attribute is still cleared normally. ``st_file_attributes`` + # exists only on Windows, so read it dynamically instead of through the + # cross-platform ``stat_result`` type. + attributes = getattr(status, "st_file_attributes", 0) + return stat.S_ISDIR(status.st_mode) and bool( + attributes & _WINDOWS_REPARSE_POINT + ) + + +def _clear_windows_read_only_files(root: str) -> None: + """Clear the read-only attribute of every file git wrote under ``root``. + + Windows refuses to unlink a read-only file even though its directory is + writable, and git marks each loose object and pack read-only, so a tree that + holds a repository cannot be removed. Only Windows has that rule: POSIX + unlinks a read-only member of a writable directory. Links and junctions are + skipped so this walk cannot reach a target outside the owned tree, and a + file with more than one link is left untouched because a hard link shares + the file (and its read-only attribute) with another directory entry that may + belong to another owner; ``rmtree`` then fails closed. + """ + for directory, subdirectories, files in os.walk(root): + subdirectories[:] = [ + name + for name in subdirectories + if not _is_link_or_junction(os.path.join(directory, name)) + ] + for name in files: + target = os.path.join(directory, name) + if _is_link_or_junction(target): + continue + try: + status = os.stat(target) + if status.st_nlink > 1: + # A hard link is another directory entry for the same file, + # and git uses them for a local clone. Clearing the + # read-only attribute here would also change the source + # repository object outside the owned tree, so leave it for + # rmtree to fail closed instead. + continue + if not status.st_mode & stat.S_IWRITE: + os.chmod(target, status.st_mode | stat.S_IWRITE) + except OSError: + continue + + +def remove_owned_tree(path: str | Path) -> None: + """Delete an owned, previously isolated tree while tolerating host attributes. + + The filesystem sibling of :func:`terminate_process_tree`: a smoke fixture + root or frozen workspace is created by one owner, filled with host fixtures, + and removed by the same owner. A fixture that contains a git repository + cannot be removed by a plain ``shutil.rmtree`` on Windows, where every loose + object git wrote carries the read-only attribute, so teardown fails with + ``PermissionError`` [WinError 5] and the whole tree is left behind. The + attribute is cleared first on Windows only; every other host already removes + a read-only member of a writable directory unchanged. A root that is itself + a link or junction is refused before any attribute is cleared: ``os.walk`` + would start at the link target, and ``shutil.rmtree`` only refuses the link + after that walk has already changed permissions outside the owned tree. + Ownership still stays with the caller: a removal that cannot complete raises, + so a caller that must retain the tree for diagnosis can keep observing it. + """ + root = os.fspath(path) + if _is_link_or_junction(root): + raise OSError(f"refusing to remove linked tree root: {root}") + if os.name == "nt": + _clear_windows_read_only_files(root) + shutil.rmtree(root) + + def run_capped_process( argv: Sequence[str], *, diff --git a/tests/canary/test_smoke_fixture_cleanup.py b/tests/canary/test_smoke_fixture_cleanup.py new file mode 100644 index 0000000000..2f3daa460b --- /dev/null +++ b/tests/canary/test_smoke_fixture_cleanup.py @@ -0,0 +1,176 @@ +from __future__ import annotations + +from pathlib import Path +import os +import stat +import sys +from typing import Any + +import pytest + +from loopx.canary import runner +from loopx.extensions.process_runtime import remove_owned_tree + + +def _read_only_fixture_argv() -> list[str]: + """argv for a check that leaves a read-only git-shaped object behind. + + The runner points every temp variable at its own per-check root, so the child + writes the object exactly where the runner later has to remove it. ``0o444`` + sets the Windows read-only attribute, as git does for every loose object. + """ + return [ + sys.executable, + "-c", + "import os, pathlib\n" + "root = pathlib.Path(os.environ['TMPDIR'])\n" + "fixture = root / 'loopx-catalog-canary-smoke-fixture' / 'selector-repo'\n" + "(fixture / '.git' / 'objects' / '39').mkdir(parents=True)\n" + "object_file = fixture / '.git' / 'objects' / '39' / '6a2d80dac5ee96f7f8e1b120f2358ae0fcda'\n" + "object_file.write_bytes(b'blob')\n" + "os.chmod(object_file, 0o444)\n" + "print(root)\n", + ] + + +def _read_only_fixture_check(monkeypatch: pytest.MonkeyPatch) -> dict[str, Any]: + argv = _read_only_fixture_argv() + normalized: dict[str, Any] = { + "ok": True, + "command": "python3 examples/read-only-fixture-smoke.py", + "argv": argv, + "display_argv": ["python3", "examples/read-only-fixture-smoke.py"], + "injected_args": [], + "script": "examples/read-only-fixture-smoke.py", + } + monkeypatch.setattr( + runner, + "normalize_canary_command", + lambda command, repo_root=None: dict(normalized), + ) + return {"command": normalized["command"], "reason": "read-only fixture check"} + + +def _fixture_root(result: dict[str, Any]) -> Path: + root = Path(str(result["stdout_tail"]).strip().splitlines()[-1]) + assert root.name.startswith("loopx-smoke-") + return root + + +def test_a_passing_check_survives_a_read_only_fixture_member( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A check whose command exited 0 stays passed, and its fixture is removed. + + A plain ``shutil.rmtree`` of the fixture root failed on native Windows with + ``PermissionError`` [WinError 5], so the runner recorded ``returncode: 0`` + together with ``status: failed`` and retained the tree. + """ + check = _read_only_fixture_check(monkeypatch) + + result = runner.run_canary_smoke_check(check, timeout_seconds=60) + + assert result["returncode"] == 0 + assert "fixture cleanup failed" not in str(result.get("reason") or "") + assert result["status"] == "passed" + assert result["ok"] is True + assert not _fixture_root(result).exists() + + +def test_the_runner_still_reports_cleanup_that_cannot_be_confirmed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The teardown seam stays fail-closed: an unconfirmed removal retains the tree. + + This pins the contract the fix must not loosen — if the owned tree cannot be + removed, the check is reported failed rather than passed and its fixture is + kept for diagnosis. + """ + check = _read_only_fixture_check(monkeypatch) + + def unconfirmed(_root: Path) -> None: + raise RuntimeError("managed runtime still exiting") + + monkeypatch.setattr(runner, "remove_owned_tree", unconfirmed) + + result = runner.run_canary_smoke_check(check, timeout_seconds=60) + + assert result["returncode"] == 0 + assert result["ok"] is False + assert str(result.get("reason") or "").startswith("fixture cleanup failed; retained") + retained = _fixture_root(result) + assert retained.exists() + remove_owned_tree(retained) + + +def _hard_link_fixture_check( + monkeypatch: pytest.MonkeyPatch, + external_object: Path, +) -> dict[str, Any]: + """A check that hard-links an owned fixture entry to an external object. + + ``git clone --local`` does the same for loose objects, so removing the fixture + must not clear the read-only attribute of the shared file. + """ + monkeypatch.setenv("LOOPX_TEST_EXTERNAL_OBJECT", str(external_object)) + argv = [ + sys.executable, + "-c", + "import os, pathlib\n" + "root = pathlib.Path(os.environ['TMPDIR'])\n" + "os.link(os.environ['LOOPX_TEST_EXTERNAL_OBJECT'], root / 'object')\n" + "print(root)\n", + ] + normalized: dict[str, Any] = { + "ok": True, + "command": "python3 examples/hard-link-fixture-smoke.py", + "argv": argv, + "display_argv": ["python3", "examples/hard-link-fixture-smoke.py"], + "injected_args": [], + "script": "examples/hard-link-fixture-smoke.py", + } + monkeypatch.setattr( + runner, + "normalize_canary_command", + lambda command, repo_root=None: dict(normalized), + ) + return {"command": normalized["command"], "reason": "hard link fixture check"} + + +def test_a_shared_hard_link_is_not_rewritten_outside_the_fixture( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + """A cleanup that cannot avoid touching a shared object fails closed. + + Windows cannot remove a read-only hard link without clearing the attribute of + the shared inode, so the only safe outcome is a reported cleanup failure with + the tree retained; the external object keeps its bytes and read-only bit. + """ + source = tmp_path / "source-object" + source.write_bytes(b"blob") + os.chmod(source, stat.S_IREAD) + check = _hard_link_fixture_check(monkeypatch, source) + + result = runner.run_canary_smoke_check(check, timeout_seconds=60) + + try: + assert result["returncode"] == 0 + if os.name == "nt": + assert result["ok"] is False + assert str(result.get("reason") or "").startswith( + "fixture cleanup failed; retained" + ) + else: + assert result["status"] == "passed" + assert not _fixture_root(result).exists() + assert source.read_bytes() == b"blob" + assert os.stat(source).st_mode & stat.S_IWRITE == 0 + finally: + if os.name == "nt": + retained = _fixture_root(result) + if retained.exists(): + # Release the shared attribute only for test cleanup, after the + # assertions above proved teardown did not do it. + os.chmod(source, stat.S_IWRITE) + remove_owned_tree(retained) diff --git a/tests/capabilities/test_issue_fix_temporary_workspace.py b/tests/capabilities/test_issue_fix_temporary_workspace.py new file mode 100644 index 0000000000..db14d75532 --- /dev/null +++ b/tests/capabilities/test_issue_fix_temporary_workspace.py @@ -0,0 +1,28 @@ +from __future__ import annotations + +import os +from pathlib import Path +import stat + +from loopx.capabilities.issue_fix import acceptance_loop + + +def test_temporary_git_workspace_teardown_removes_read_only_objects( + tmp_path: Path, +) -> None: + """The packet's own temporary repository is removable on every host. + + Git marks every loose object read-only, and Windows refuses to unlink a + read-only file, so the teardown used to raise ``PermissionError`` + [WinError 5] from its ``finally`` block after five pointless retries. + """ + workspace = tmp_path / "loopx-issue-fix-git-fixture" + objects = workspace / ".git" / "objects" / "32" + objects.mkdir(parents=True) + object_file = objects / "667d2c685aa01495f7571c1a13fe83ad1525f3" + object_file.write_bytes(b"blob") + os.chmod(object_file, stat.S_IREAD) + + acceptance_loop._remove_temporary_git_workspace(workspace) + + assert not workspace.exists() diff --git a/tests/extensions/test_process_runtime.py b/tests/extensions/test_process_runtime.py index bfb90839f1..eeee1505c0 100644 --- a/tests/extensions/test_process_runtime.py +++ b/tests/extensions/test_process_runtime.py @@ -4,6 +4,7 @@ import os from pathlib import Path import signal +import stat import subprocess import sys import time @@ -12,7 +13,11 @@ import pytest import loopx.extensions.process_runtime as process_runtime -from loopx.extensions.process_runtime import run_capped_process, terminate_process_tree +from loopx.extensions.process_runtime import ( + remove_owned_tree, + run_capped_process, + terminate_process_tree, +) @pytest.mark.skipif(os.name != "posix", reason="POSIX process-group regression") @@ -423,3 +428,113 @@ def test_no_execution_deadline_preserves_completion_and_output_budget(): stdin=b"", timeout_seconds=None, output_limit_bytes=1024, ) assert limited.failure_kind == "response_too_large" + + +def test_remove_owned_tree_removes_a_read_only_member(tmp_path: Path) -> None: + # Git marks every loose object it writes read-only, and Windows refuses to + # unlink a read-only file, which stranded the whole fixture root. + object_directory = tmp_path / ".git" / "objects" / "39" + object_directory.mkdir(parents=True) + object_file = object_directory / "6a2d80dac5ee96f7f8e1b120f2358ae0fcda" + object_file.write_bytes(b"blob") + os.chmod(object_file, stat.S_IREAD) + + remove_owned_tree(tmp_path) + + assert not tmp_path.exists() + + +def _write_read_only_fixture(root: Path) -> Path: + """A git-shaped read-only object, the member the Windows pass rewrites.""" + root.mkdir() + fixture = root / "object" + fixture.write_bytes(b"blob") + os.chmod(fixture, stat.S_IREAD) + return fixture + + +def _assert_linked_target_untouched(target: Path, fixture: Path) -> None: + assert target.is_dir() + assert fixture.read_bytes() == b"blob" + assert os.stat(fixture).st_mode & stat.S_IWRITE == 0 + + +def test_remove_owned_tree_refuses_a_symlinked_root_without_touching_its_target( + tmp_path: Path, +) -> None: + # os.walk starts at the root even when it is a link, so the Windows + # attribute pass would chmod the link target before shutil.rmtree refused + # the link. The rejection has to happen while the target is still untouched. + target = tmp_path / "external" + fixture = _write_read_only_fixture(target) + root = tmp_path / "owned-root" + try: + root.symlink_to(target, target_is_directory=True) + except (NotImplementedError, OSError) as error: + pytest.skip(f"cannot create a directory symlink: {error}") + + with pytest.raises(OSError, match="linked tree root"): + remove_owned_tree(root) + + assert root.is_symlink() + _assert_linked_target_untouched(target, fixture) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows junction regression") +def test_remove_owned_tree_refuses_a_junction_root_without_touching_its_target( + tmp_path: Path, +) -> None: + # Junctions are not symlinks, so os.path.islink misses them and os.walk + # descends into the target; they are the unprivileged Windows equivalent of + # the directory-symlink root the guard must refuse. + target = tmp_path / "external" + fixture = _write_read_only_fixture(target) + root = tmp_path / "owned-root" + created = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(root), str(target)], + capture_output=True, text=True, check=False, + ) + assert created.returncode == 0, created.stderr or created.stdout + + with pytest.raises(OSError, match="linked tree root"): + remove_owned_tree(root) + + assert os.path.isdir(root) + _assert_linked_target_untouched(target, fixture) + + +def test_remove_owned_tree_never_clears_a_shared_hard_link_attribute( + tmp_path: Path, +) -> None: + # git clone --local hard-links loose objects, so a read-only clone object is + # the same file as the source object. The Windows attribute pass must not + # clear its read-only bit: that would change a file outside the owned + # fixture, and a successful removal would hide the side effect. + source = tmp_path / "source" + source.mkdir() + source_object = source / "object" + source_object.write_bytes(b"blob") + os.chmod(source_object, stat.S_IREAD) + owned = tmp_path / "owned" + owned.mkdir() + try: + os.link(source_object, owned / "object") + except (NotImplementedError, OSError) as error: + pytest.skip(f"cannot create a hard link: {error}") + assert os.stat(source_object).st_nlink == 2 + + if os.name == "nt": + # Fail closed: this directory entry cannot be removed without changing + # the shared file, so the fixture is retained and the caller reports + # cleanup failure instead of mutating another owner's object. + with pytest.raises(OSError): + remove_owned_tree(owned) + assert owned.exists() + else: + # POSIX unlinks the read-only directory entry without touching the + # inode or its other link. + remove_owned_tree(owned) + assert not owned.exists() + + assert source_object.read_bytes() == b"blob" + assert os.stat(source_object).st_mode & stat.S_IWRITE == 0