From f1deb01ee5533f9331648b5d521c4c15a9690fde Mon Sep 17 00:00:00 2001 From: JasonBuildAI Date: Sat, 17 Oct 2026 12:37:31 +0800 Subject: [PATCH 1/5] fix(canary): keep a passing smoke's verdict when its fixture holds read-only git objects On native Windows the canary smoke runner could not remove its own per-check fixture root whenever the smoke created a git repository inside it: git marks every loose object and pack read-only, and Windows refuses to unlink a read-only file, so `shutil.rmtree` raised `PermissionError` [WinError 5]. The runner then recorded the check as failed although its command had exited 0, retained the tree, and reported the cleanup error as the check's own reason, so a contributor could not tell a real regression from harness noise. Owned tree removal now goes through `extensions.process_runtime.remove_owned_tree`, which clears the Windows read-only attribute of the files in the tree before removing it. POSIX behaviour is unchanged: there a read-only member of a writable directory never blocks unlink, so no extra code runs. A removal that still cannot complete raises, so a caller that must retain the tree for diagnosis keeps observing it. The same host behaviour aborted the issue-fix packet's temporary git workspace teardown from its `finally` block after five pointless retries, and in two public reviewer smokes it masked a real failure behind the cleanup error; those call sites now use the same owner. Refs #6710 Signed-off-by: JasonBuildAI --- ...issue-fix-reviewer-recommendation-smoke.py | 4 +- examples/issue-fix-reviewer-request-smoke.py | 4 +- loopx/canary/runner.py | 17 +++++-- .../capabilities/issue_fix/acceptance_loop.py | 10 ++++- loopx/extensions/process_runtime.py | 44 +++++++++++++++++++ 5 files changed, 70 insertions(+), 9 deletions(-) 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..18464a7dcf 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,48 @@ def cleanup() -> None: closed = True return cleanup +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 are skipped so + this walk cannot reach a target outside the owned tree. + """ + for directory, _subdirectories, files in os.walk(root): + for name in files: + target = os.path.join(directory, name) + if os.path.islink(target): + continue + try: + mode = os.stat(target).st_mode + if not mode & stat.S_IWRITE: + os.chmod(target, 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. 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 os.name == "nt": + _clear_windows_read_only_files(root) + shutil.rmtree(root) + + def run_capped_process( argv: Sequence[str], *, From 0da518a2eda845f50e5d454df762dcb29785d1a9 Mon Sep 17 00:00:00 2001 From: JasonBuildAI Date: Sat, 17 Oct 2026 12:37:32 +0800 Subject: [PATCH 2/5] test: pin Windows read-only fixture teardown at both owners The canary-runner and issue-fix teardown regressions fail on native Windows with `PermissionError` [WinError 5] when the fix is absent, and pass on POSIX either way because a read-only member never blocks unlink there; the pre-fix behaviour was reproduced by substituting a plain `shutil.rmtree` at the two call sites. Refs #6710 Signed-off-by: JasonBuildAI --- tests/canary/test_smoke_fixture_cleanup.py | 101 ++++++++++++++++++ .../test_issue_fix_temporary_workspace.py | 28 +++++ tests/extensions/test_process_runtime.py | 21 +++- 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 tests/canary/test_smoke_fixture_cleanup.py create mode 100644 tests/capabilities/test_issue_fix_temporary_workspace.py diff --git a/tests/canary/test_smoke_fixture_cleanup.py b/tests/canary/test_smoke_fixture_cleanup.py new file mode 100644 index 0000000000..bd0c6f87c4 --- /dev/null +++ b/tests/canary/test_smoke_fixture_cleanup.py @@ -0,0 +1,101 @@ +from __future__ import annotations + +from pathlib import Path +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) 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..7f4bb52948 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,17 @@ 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() From b0a793f9c1949c2ae91eba00c3dfb2c651dd3f6e Mon Sep 17 00:00:00 2001 From: JasonBuildAI Date: Sat, 17 Oct 2026 13:23:42 +0800 Subject: [PATCH 3/5] ci(windows): run the read-only fixture teardown regressions on Windows The new regression suites pass on POSIX with or without the fix, because unlink there ignores the read-only bit, and the curated `windows-powershell` lane ran none of them, so the native-Windows behaviour they encode had no guard on the host where it reproduces. Point that lane's qualification at them. Refs #6710 Signed-off-by: JasonBuildAI --- .github/workflows/python-tests.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/python-tests.yml b/.github/workflows/python-tests.yml index 173414ba10..2baa475d3e 100644 --- a/.github/workflows/python-tests.yml +++ b/.github/workflows/python-tests.yml @@ -819,6 +819,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 From 5c7f2d9c13db3d7b5a10d84988cd9bb87b60340a Mon Sep 17 00:00:00 2001 From: JasonBuildAI Date: Sat, 17 Oct 2026 17:34:19 +0800 Subject: [PATCH 4/5] fix(canary): refuse a linked fixture root before clearing attributes os.walk starts at a root that is itself a directory symlink or Windows junction, so the Windows pre-pass could clear the read-only attribute of a file in the link target before shutil.rmtree refused the link. Refuse a linked root first, and skip linked directories inside the walk, so teardown never changes permissions outside the owned tree. Cover the rejection with a no-external-mutation regression for both a directory symlink and a native Windows junction. Signed-off-by: JasonBuildAI --- loopx/extensions/process_runtime.py | 54 +++++++++++++++++++--- tests/extensions/test_process_runtime.py | 59 ++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 7 deletions(-) diff --git a/loopx/extensions/process_runtime.py b/loopx/extensions/process_runtime.py index 18464a7dcf..ff67d74aa0 100644 --- a/loopx/extensions/process_runtime.py +++ b/loopx/extensions/process_runtime.py @@ -241,19 +241,54 @@ 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 are skipped so - this walk cannot reach a target outside the owned tree. + 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. """ - for directory, _subdirectories, files in os.walk(root): + 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 os.path.islink(target): + if _is_link_or_junction(target): continue try: mode = os.stat(target).st_mode @@ -273,11 +308,16 @@ def remove_owned_tree(path: str | Path) -> None: 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. 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. + 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) diff --git a/tests/extensions/test_process_runtime.py b/tests/extensions/test_process_runtime.py index 7f4bb52948..13ccf698b6 100644 --- a/tests/extensions/test_process_runtime.py +++ b/tests/extensions/test_process_runtime.py @@ -442,3 +442,62 @@ def test_remove_owned_tree_removes_a_read_only_member(tmp_path: Path) -> None: 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) From 803c0e242fce52aeaf0e97f087a7c2f39a1e404a Mon Sep 17 00:00:00 2001 From: JasonBuildAI Date: Sat, 17 Oct 2026 18:46:25 +0800 Subject: [PATCH 5/5] fix(canary): keep shared hard-linked objects out of the attribute pass A git clone --local hard-links loose objects, and Windows keeps the read-only attribute on the shared file rather than on one directory entry. The Windows pre-pass therefore cleared the source repository object too, while the teardown still reported success. Leave every file with more than one link untouched so rmtree fails closed and the caller retains the fixture instead of mutating an object outside it. Cover the fixture entry and the real canary runner with a hard-link regression that asserts the external object keeps its bytes and read-only attribute. Signed-off-by: JasonBuildAI --- loopx/extensions/process_runtime.py | 18 ++++-- tests/canary/test_smoke_fixture_cleanup.py | 75 ++++++++++++++++++++++ tests/extensions/test_process_runtime.py | 37 +++++++++++ 3 files changed, 126 insertions(+), 4 deletions(-) diff --git a/loopx/extensions/process_runtime.py b/loopx/extensions/process_runtime.py index ff67d74aa0..4a5dcfd026 100644 --- a/loopx/extensions/process_runtime.py +++ b/loopx/extensions/process_runtime.py @@ -278,7 +278,10 @@ def _clear_windows_read_only_files(root: str) -> None: 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. + 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[:] = [ @@ -291,9 +294,16 @@ def _clear_windows_read_only_files(root: str) -> None: if _is_link_or_junction(target): continue try: - mode = os.stat(target).st_mode - if not mode & stat.S_IWRITE: - os.chmod(target, mode | stat.S_IWRITE) + 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 diff --git a/tests/canary/test_smoke_fixture_cleanup.py b/tests/canary/test_smoke_fixture_cleanup.py index bd0c6f87c4..2f3daa460b 100644 --- a/tests/canary/test_smoke_fixture_cleanup.py +++ b/tests/canary/test_smoke_fixture_cleanup.py @@ -1,6 +1,8 @@ from __future__ import annotations from pathlib import Path +import os +import stat import sys from typing import Any @@ -99,3 +101,76 @@ def unconfirmed(_root: Path) -> None: 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/extensions/test_process_runtime.py b/tests/extensions/test_process_runtime.py index 13ccf698b6..eeee1505c0 100644 --- a/tests/extensions/test_process_runtime.py +++ b/tests/extensions/test_process_runtime.py @@ -501,3 +501,40 @@ def test_remove_owned_tree_refuses_a_junction_root_without_touching_its_target( 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