diff --git a/tests/conftest.py b/tests/conftest.py index 0b7fbbb..4ae00f5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re import subprocess from pathlib import Path @@ -9,6 +10,32 @@ REPO_ROOT = Path(__file__).parent.parent +# The one copier failure the suite tolerates (DOT-606), matched as a whole rather than as +# loose substrings. The `OSError` line must itself name a path under copier's own temp +# clone: a render error's traceback also mentions that directory — every template frame +# lives inside it — so `"copier._vcs.clone" in stderr` is nearly vacuous on its own. +_CLONE_CLEANUP_RE = re.compile( + r"^OSError: \[Errno \d+\] Directory not empty: .*copier\._vcs\.clone\.", + re.MULTILINE, +) + +# ...and it must have been raised *by* the cleanup, not merely alongside it. +_CLEANUP_FRAME = ", in _cleanup" + +# A chained traceback means something failed first and the cleanup crash rode along on the +# unwind. That is the dangerous shape: a partial render whose exception is followed by the +# cleanup `OSError`, which would otherwise satisfy every check above. +_CHAINED_EXCEPTION_MARKERS = ( + "During handling of the above exception", + "The above exception was the direct cause", +) + +# The race is intermittent — the same dirty tree renders fine on the next attempt — so a +# re-render is the actual fix and stderr matching only decides whether to spend one. Three +# is chosen against a measured hit rate around one render in ten; a run that loses the race +# three times running fails, which is the safe direction. +_MAX_RENDER_ATTEMPTS = 3 + @pytest.fixture def copier_defaults() -> dict: @@ -69,13 +96,64 @@ def generate_project( else: cmd.extend(["-d", f"{key}={value}"]) - result = subprocess.run(cmd, capture_output=True, text=True, check=False) - if result.returncode != 0: - pytest.fail(f"Copier failed: {result.stderr}") + for attempt in range(1, _MAX_RENDER_ATTEMPTS + 1): + result = subprocess.run(cmd, capture_output=True, text=True, check=False) + if result.returncode == 0: + return tmp_path + # A non-zero exit is never accepted as success. The cleanup race only buys another + # attempt; if copier keeps failing, the last stderr is what the test reports. + if attempt < _MAX_RENDER_ATTEMPTS and _is_retryable_cleanup_race(result.stderr, tmp_path): + continue + pytest.fail(f"Copier failed (attempt {attempt}/{_MAX_RENDER_ATTEMPTS}): {result.stderr}") return tmp_path +def _is_retryable_cleanup_race(stderr: str, dst: Path) -> bool: + """True if copier's exit looks like the temp-clone cleanup race, so re-rendering is worth a shot (DOT-606). + + On a dirty working tree copier takes its dirty-file overlay path, which does extra git + work inside the temp clone it made of this repo. Something still holds a handle under + that clone's `.git` when `_cleanup` calls `rmtree(..., ignore_errors=False)`, so copier + exits non-zero *after* every file has been written. Observed on macOS with copier 9.17.x. + + The effect is a test suite that is green in CI (which always checks out clean) and red + for anyone mid-change — with the failure landing on whichever test happens to render + first, so it reads as flakiness rather than as one deterministic bug. Re-running the + "failed" test alone usually passes, which sends triage down the wrong path entirely. + + This decides whether to *retry*, never whether to accept a failure. That distinction is + what bounds the damage of a wrong answer: a false positive costs one extra render and + then fails with copier's real stderr anyway, and a false negative fails immediately — + which is what would have happened without any of this. Nothing is ever suppressed, so + no amount of cleverness in classifying stderr is load-bearing for correctness. + + Still matched tightly, to keep pointless retries rare. All four must hold: + + 1. The final exception is an `OSError` naming a path under copier's own temp clone. + 2. It was raised from a `_cleanup` frame — the cleanup is what failed, not something + that merely happened to mention the same directory. + 3. Nothing failed before it. A chained traceback is how a partial render presents: its + own exception, then the cleanup `OSError` raised while unwinding. + 4. The destination actually received a render. + + Independent substring checks are not enough for (1) and (2): every template frame in a + Jinja render error lives *inside* the temp clone, so a genuine render failure can put + `copier._vcs.clone` and `rmtree` in stderr on unrelated lines while `pyproject.toml` — + written early — already exists in the destination. + + If copier renames `_cleanup`, this stops matching and the race stops being retried. + That is the intended direction to fail in. + """ + if not _CLONE_CLEANUP_RE.search(stderr): + return False + if _CLEANUP_FRAME not in stderr: + return False + if any(marker in stderr for marker in _CHAINED_EXCEPTION_MARKERS): + return False + return (dst / "pyproject.toml").is_file() + + def _cache_key(answers: dict, project_type: str) -> str: """Create a stable cache key from answers dict and project type.""" merged = {**answers, "project_type": project_type} diff --git a/tests/test_template.py b/tests/test_template.py index d4e808a..7f163a8 100644 --- a/tests/test_template.py +++ b/tests/test_template.py @@ -13,7 +13,7 @@ import pytest import yaml -from conftest import REPO_ROOT, generate_project +from conftest import REPO_ROOT, _is_retryable_cleanup_race, generate_project if TYPE_CHECKING: from pathlib import Path @@ -2652,3 +2652,88 @@ def test_semantic_release_writes_gitlab_urls_for_selfhosted(self, tmp_path: Path f"Underscored repo slug `die_zeit` leaked into CHANGELOG.md — semantic-release " f"appears to be using python_package_import_name instead of the repo slug:\n{changelog}" ) + + +class TestTempCloneCleanupTolerance: + """Pin the boundary of `conftest._is_retryable_cleanup_race` (DOT-606). + + The helper decides whether a failed render is worth re-running, so a wrong answer costs + a wasted render or an early failure — never a swallowed one. It is still worth pinning: + widen it and the suite burns three renders on every genuinely broken template; narrow it + and DOT-606's dirty-tree failures stop being retried and come back. + + The integration tests cannot cover this. They only see whichever exit copier produces on + the machine running them, and on a clean tree that is exit 0 with the helper never + consulted at all. + """ + + # Trimmed from a real failure: dirty tree, `copier copy -r HEAD`, copier 9.17.1 on + # macOS. Frame paths shortened, structure and the final line kept verbatim. + CLEANUP_ONLY = """Traceback (most recent call last): + File "/venv/copier/_template.py", line 240, in _cleanup + rmtree(temp_clone, ignore_errors=False, onexc=handle_remove_readonly) + File "/python/shutil.py", line 753, in _rmtree_safe_fd_step + os.rmdir(name, dir_fd=dirfd) +OSError: [Errno 66] Directory not empty: PosixPath('/tmp/copier._vcs.clone.ulvgwzp7') +""" + + # A render that died partway, with the cleanup crash chained onto the unwind. Every + # loose substring the old matcher looked for is present, and `pyproject.toml` was + # written before the failure — so only the chaining marker separates this from benign. + PARTIAL_RENDER_THEN_CLEANUP = """Traceback (most recent call last): + File "/venv/copier/_main.py", line 850, in _render_file + raise UndefinedError(msg) +jinja2.exceptions.UndefinedError: 'project_name' is undefined + +During handling of the above exception, another exception occurred: + +Traceback (most recent call last): + File "/venv/copier/_template.py", line 240, in _cleanup + rmtree(temp_clone, ignore_errors=False, onexc=handle_remove_readonly) +OSError: [Errno 66] Directory not empty: PosixPath('/tmp/copier._vcs.clone.ulvgwzp7') +""" + + # A pure render failure. Mentions the clone directory (every template frame lives + # inside it) and the word `rmtree`, but nothing here is a cleanup failure. + RENDER_ERROR_ONLY = """Traceback (most recent call last): + File "/tmp/copier._vcs.clone.ulvgwzp7/project/scripts/rmtree_helper.py", line 3 + {{ project_name } +jinja2.exceptions.TemplateSyntaxError: unexpected '}' +""" + + @staticmethod + def _with_render(tmp_path: Path) -> Path: + """A destination that received a project: the render-succeeded half of the gate.""" + (tmp_path / "pyproject.toml").write_text('[project]\nname = "rendered"\n') + return tmp_path + + def test_accepts_a_cleanup_only_failure(self, tmp_path: Path) -> None: + """The one shape this exists for: render finished, `_cleanup` could not rmtree.""" + assert _is_retryable_cleanup_race(self.CLEANUP_ONLY, self._with_render(tmp_path)) + + def test_rejects_partial_render_with_chained_cleanup_failure(self, tmp_path: Path) -> None: + """The dangerous case: a real render error whose unwind also trips the cleanup. + + Every individual token the matcher keys on is present, and the destination holds a + `pyproject.toml` written before the failure. Suppressing this would run the whole + suite against a half-rendered project. + """ + assert not _is_retryable_cleanup_race(self.PARTIAL_RENDER_THEN_CLEANUP, self._with_render(tmp_path)) + + def test_rejects_render_error_that_merely_mentions_the_clone(self, tmp_path: Path) -> None: + """`copier._vcs.clone` and `rmtree` in stderr prove nothing on their own.""" + assert not _is_retryable_cleanup_race(self.RENDER_ERROR_ONLY, self._with_render(tmp_path)) + + def test_rejects_cleanup_failure_outside_copiers_temp_clone(self, tmp_path: Path) -> None: + """Same exception, a directory copier did not create — not ours to excuse.""" + stderr = self.CLEANUP_ONLY.replace("copier._vcs.clone.ulvgwzp7", "some-other-dir") + assert not _is_retryable_cleanup_race(stderr, self._with_render(tmp_path)) + + def test_rejects_when_the_failure_was_not_raised_by_cleanup(self, tmp_path: Path) -> None: + """Bind the OSError to the `_cleanup` frame, not just to the clone path.""" + stderr = self.CLEANUP_ONLY.replace(", in _cleanup", ", in _render_file") + assert not _is_retryable_cleanup_race(stderr, self._with_render(tmp_path)) + + def test_rejects_when_nothing_was_rendered(self, tmp_path: Path) -> None: + """The load-bearing gate: cleanup noise never excuses an empty destination.""" + assert not _is_retryable_cleanup_race(self.CLEANUP_ONLY, tmp_path)