Skip to content

test: tolerate copier's temp-clone cleanup race on a dirty tree - #348

Open
ichoosetoaccept wants to merge 4 commits into
no-blanket-chmod-scriptsfrom
tolerate-clone-cleanup-race
Open

test: tolerate copier's temp-clone cleanup race on a dirty tree#348
ichoosetoaccept wants to merge 4 commits into
no-blanket-chmod-scriptsfrom
tolerate-clone-cleanup-race

Conversation

@ichoosetoaccept

@ichoosetoaccept ichoosetoaccept commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

generate_project in tests/conftest.py now tolerates one specific copier exit: a successful render whose temp-clone cleanup crashed.

Why

poe test failed whenever the working tree was dirty and passed when it was clean. One unrelated newline reproduced it. CI never saw it — CI always checks out clean — so the suite was green exactly when nobody needed it and red exactly when somebody was mid-change.

On a dirty tree copier takes its dirty-file overlay path, which does extra git work inside the temp clone it makes 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 already been written. The render succeeds; only the cleanup fails.

It reads as flakiness and is not. The failure lands on whichever test happens to render first, so the count and identity of "failing" tests vary per run, and re-running one in isolation usually passes — which sends triage toward "just a flake" and then toward "it started when I changed X". Both are wrong.

Narrow on purpose, in three ways: stderr must name a path copier itself calls copier._vcs.clone.*, it must carry an rmtree/Errno 66 signature, and the destination must actually contain a rendered pyproject.toml. A render that genuinely failed writes no pyproject.toml, and every other non-zero exit still fails the test with copier's own stderr. A broader "ignore non-zero exits" would be a real liability in a suite whose entire job is asserting on rendered output.

Test plan

  • Fast suite on a dirty tree: 202 passed. The same tree previously gave 2 failed, 13 passed in TestTemplateUpdateCheck alone, with the failures on tests unrelated to the edit.
  • Fast suite on a clean tree: unchanged, the tolerance path is never entered.

Closes DOT-606

Greptile Summary

The fixture now retries a narrowly classified temporary-clone cleanup failure instead of accepting any nonzero Copier result. Focused tests cover the accepted cleanup traceback and its rejection gates.

  • Adds a maximum of three render attempts for the intermittent dirty-tree cleanup race.
  • Requires a cleanup-frame traceback, Copier temporary-clone path, no chained exception, and rendered pyproject.toml before retrying.
  • Adds regression coverage based on representative cleanup and render-error tracebacks.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the current implementation only returns after a successful Copier exit, and the previously reported coverage and partial-render suppression issues are fixed.

Important Files Changed

Filename Overview
tests/conftest.py Replaces failure suppression with bounded retry behavior while preserving failure for every unsuccessful final attempt.
tests/test_template.py Adds focused coverage for the retry classifier’s accepted traceback and rejection boundaries.

Reviews (6): Last reviewed commit: "fix(test): retry the cleanup race instea..." | Re-trigger Greptile

@ichoosetoaccept

ichoosetoaccept commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

@linear-code

linear-code Bot commented Aug 5, 2026

Copy link
Copy Markdown

DOT-606

Comment thread tests/conftest.py Outdated
ichoosetoaccept added a commit that referenced this pull request Aug 5, 2026
`_is_temp_clone_cleanup_failure` is the only place in the suite that suppresses
a non-zero copier exit, and it shipped with nothing testing where it stops. Both
directions of drift are silent: widen it and a genuine render failure passes as
a green test; narrow it and the dirty-tree failures it exists to absorb come
back. The integration tests cannot catch either, because they only see whichever
exit copier produces on the machine running them -- on a clean tree that is exit
0 and the helper is never consulted.

Covers each accepted marker in isolation (`Directory not empty`, `rmtree`,
`Errno 66`) so no case leans on another's signature, plus all three rejection
gates: an OSError outside copier's own temp clone, a real failure inside the
clone with no cleanup signature, and cleanup noise over a destination that never
received a render.

Confirmed to discriminate: replacing the rendered-`pyproject.toml` gate with
`return True` fails `test_rejects_when_nothing_was_rendered`.

Reported by Greptile on #348.
@ichoosetoaccept
ichoosetoaccept force-pushed the tolerate-clone-cleanup-race branch from d829645 to c87ee6f Compare August 5, 2026 19:36
Comment thread tests/conftest.py Outdated
ichoosetoaccept added a commit that referenced this pull request Aug 5, 2026
The matcher looked for three independent substrings anywhere in stderr:
`copier._vcs.clone`, one of `Directory not empty`/`rmtree`/`Errno 66`, and a
rendered `pyproject.toml` in the destination. Every one of those can be true of
a *failed* render.

Copier renders from the temp clone, so every template frame in a Jinja
traceback carries a `copier._vcs.clone.*` path -- the first check is close to
vacuous. Worse, the cleanup crash is raised while unwinding, so a real render
error and the `OSError` appear together in the same stderr, and `pyproject.toml`
is written early enough to survive a later failure. The suite would have run
against a half-rendered project and reported green.

Now all four must hold: the final `OSError` line itself names a path under
copier's temp clone, it was raised from a `_cleanup` frame, no chained-exception
marker precedes it, and the destination received a render. Chaining is the
discriminator that matters -- it is precisely how a partial render presents.

Verified against the real thing: reproduced the dirty-tree failure (copier
9.17.1, macOS), captured stderr, and confirmed the matcher accepts it. Against
the two failure shapes, the old matcher returned True for both and the new one
returns False for both:

    CLEANUP_ONLY                 old=True   new=True
    PARTIAL_RENDER_THEN_CLEANUP  old=True   new=False
    RENDER_ERROR_ONLY            old=True   new=False

Tests rewritten around trimmed copies of real tracebacks rather than synthetic
marker strings, which is what let the loose matcher look covered. Fast suite:
208 passed.

Reported by Greptile on #348.
Comment thread tests/conftest.py
ichoosetoaccept added a commit that referenced this pull request Aug 5, 2026
Greptile's follow-up is right that no stderr pattern can prove a render
completed. `raise ... from None` would drop the chaining marker the previous
commit leaned on, and in general the absence of evidence of an earlier failure
is not evidence of its absence. Copier itself has no `from None` or
`__suppress_context__` anywhere in 9.17.1, so the specific mechanism cannot fire
today -- but the objection stands on principle, and the design that answers it
is simpler than the matcher it replaces.

So stop accepting non-zero exits at all. The race is intermittent -- the same
dirty tree renders fine on the next attempt -- so a re-render is the actual
remedy, and stderr matching now only decides whether to spend one. Three
attempts against a hit rate around one render in ten; lose three in a row and
the test fails with copier's stderr, which is the safe direction.

The matcher stays tight to keep pointless retries rare, but it is no longer
load-bearing: a false positive costs one render and then fails anyway, a false
negative fails immediately, and neither can hide a partial render. Renamed to
`_is_retryable_cleanup_race` to stop implying it certifies success.

Verified a genuine copier failure (a missing required answer) aborts on attempt
1/3 with copier's full traceback, no retry. Fast suite on a dirty tree: 208
passed.

Reported by Greptile on #348.
@ichoosetoaccept
ichoosetoaccept force-pushed the no-blanket-chmod-scripts branch from b61c3ad to 2dbd7aa Compare August 5, 2026 23:34
ichoosetoaccept added a commit that referenced this pull request Aug 5, 2026
`_is_temp_clone_cleanup_failure` is the only place in the suite that suppresses
a non-zero copier exit, and it shipped with nothing testing where it stops. Both
directions of drift are silent: widen it and a genuine render failure passes as
a green test; narrow it and the dirty-tree failures it exists to absorb come
back. The integration tests cannot catch either, because they only see whichever
exit copier produces on the machine running them -- on a clean tree that is exit
0 and the helper is never consulted.

Covers each accepted marker in isolation (`Directory not empty`, `rmtree`,
`Errno 66`) so no case leans on another's signature, plus all three rejection
gates: an OSError outside copier's own temp clone, a real failure inside the
clone with no cleanup signature, and cleanup noise over a destination that never
received a render.

Confirmed to discriminate: replacing the rendered-`pyproject.toml` gate with
`return True` fails `test_rejects_when_nothing_was_rendered`.

Reported by Greptile on #348.
ichoosetoaccept added a commit that referenced this pull request Aug 5, 2026
The matcher looked for three independent substrings anywhere in stderr:
`copier._vcs.clone`, one of `Directory not empty`/`rmtree`/`Errno 66`, and a
rendered `pyproject.toml` in the destination. Every one of those can be true of
a *failed* render.

Copier renders from the temp clone, so every template frame in a Jinja
traceback carries a `copier._vcs.clone.*` path -- the first check is close to
vacuous. Worse, the cleanup crash is raised while unwinding, so a real render
error and the `OSError` appear together in the same stderr, and `pyproject.toml`
is written early enough to survive a later failure. The suite would have run
against a half-rendered project and reported green.

Now all four must hold: the final `OSError` line itself names a path under
copier's temp clone, it was raised from a `_cleanup` frame, no chained-exception
marker precedes it, and the destination received a render. Chaining is the
discriminator that matters -- it is precisely how a partial render presents.

Verified against the real thing: reproduced the dirty-tree failure (copier
9.17.1, macOS), captured stderr, and confirmed the matcher accepts it. Against
the two failure shapes, the old matcher returned True for both and the new one
returns False for both:

    CLEANUP_ONLY                 old=True   new=True
    PARTIAL_RENDER_THEN_CLEANUP  old=True   new=False
    RENDER_ERROR_ONLY            old=True   new=False

Tests rewritten around trimmed copies of real tracebacks rather than synthetic
marker strings, which is what let the loose matcher look covered. Fast suite:
208 passed.

Reported by Greptile on #348.
ichoosetoaccept added a commit that referenced this pull request Aug 5, 2026
Greptile's follow-up is right that no stderr pattern can prove a render
completed. `raise ... from None` would drop the chaining marker the previous
commit leaned on, and in general the absence of evidence of an earlier failure
is not evidence of its absence. Copier itself has no `from None` or
`__suppress_context__` anywhere in 9.17.1, so the specific mechanism cannot fire
today -- but the objection stands on principle, and the design that answers it
is simpler than the matcher it replaces.

So stop accepting non-zero exits at all. The race is intermittent -- the same
dirty tree renders fine on the next attempt -- so a re-render is the actual
remedy, and stderr matching now only decides whether to spend one. Three
attempts against a hit rate around one render in ten; lose three in a row and
the test fails with copier's stderr, which is the safe direction.

The matcher stays tight to keep pointless retries rare, but it is no longer
load-bearing: a false positive costs one render and then fails anyway, a false
negative fails immediately, and neither can hide a partial render. Renamed to
`_is_retryable_cleanup_race` to stop implying it certifies success.

Verified a genuine copier failure (a missing required answer) aborts on attempt
1/3 with copier's full traceback, no retry. Fast suite on a dirty tree: 208
passed.

Reported by Greptile on #348.
@ichoosetoaccept
ichoosetoaccept force-pushed the tolerate-clone-cleanup-race branch from 572e982 to 7d05f01 Compare August 5, 2026 23:35
`poe test` failed whenever the working tree was dirty and passed when it was
clean, with the failure landing on whichever test happened to render first. One
unrelated newline reproduced it. CI never saw it, because CI always checks out
clean -- so the suite was green exactly when nobody needed it and red exactly
when somebody was mid-change, with no signal separating it from a real failure.

On a dirty tree copier takes its dirty-file overlay path, which does extra git
work inside the temp clone it makes 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 already been written. The render
succeeds; only the cleanup fails.

`generate_project` now tolerates that one exit, narrowly in three ways: stderr
must name a path copier itself calls `copier._vcs.clone.*`, it must carry an
rmtree/Errno 66 signature, and the destination must actually contain a rendered
`pyproject.toml`. A render that genuinely failed writes no pyproject.toml, and
every other non-zero exit still fails the test with copier's own stderr. A
broader "ignore non-zero exits" would be a liability in a suite whose whole job
is asserting on rendered output.

Fast suite on a dirty tree: 202 passed. The same tree previously gave
2 failed, 13 passed in TestTemplateUpdateCheck alone.

Closes DOT-606
`_is_temp_clone_cleanup_failure` is the only place in the suite that suppresses
a non-zero copier exit, and it shipped with nothing testing where it stops. Both
directions of drift are silent: widen it and a genuine render failure passes as
a green test; narrow it and the dirty-tree failures it exists to absorb come
back. The integration tests cannot catch either, because they only see whichever
exit copier produces on the machine running them -- on a clean tree that is exit
0 and the helper is never consulted.

Covers each accepted marker in isolation (`Directory not empty`, `rmtree`,
`Errno 66`) so no case leans on another's signature, plus all three rejection
gates: an OSError outside copier's own temp clone, a real failure inside the
clone with no cleanup signature, and cleanup noise over a destination that never
received a render.

Confirmed to discriminate: replacing the rendered-`pyproject.toml` gate with
`return True` fails `test_rejects_when_nothing_was_rendered`.

Reported by Greptile on #348.
The matcher looked for three independent substrings anywhere in stderr:
`copier._vcs.clone`, one of `Directory not empty`/`rmtree`/`Errno 66`, and a
rendered `pyproject.toml` in the destination. Every one of those can be true of
a *failed* render.

Copier renders from the temp clone, so every template frame in a Jinja
traceback carries a `copier._vcs.clone.*` path -- the first check is close to
vacuous. Worse, the cleanup crash is raised while unwinding, so a real render
error and the `OSError` appear together in the same stderr, and `pyproject.toml`
is written early enough to survive a later failure. The suite would have run
against a half-rendered project and reported green.

Now all four must hold: the final `OSError` line itself names a path under
copier's temp clone, it was raised from a `_cleanup` frame, no chained-exception
marker precedes it, and the destination received a render. Chaining is the
discriminator that matters -- it is precisely how a partial render presents.

Verified against the real thing: reproduced the dirty-tree failure (copier
9.17.1, macOS), captured stderr, and confirmed the matcher accepts it. Against
the two failure shapes, the old matcher returned True for both and the new one
returns False for both:

    CLEANUP_ONLY                 old=True   new=True
    PARTIAL_RENDER_THEN_CLEANUP  old=True   new=False
    RENDER_ERROR_ONLY            old=True   new=False

Tests rewritten around trimmed copies of real tracebacks rather than synthetic
marker strings, which is what let the loose matcher look covered. Fast suite:
208 passed.

Reported by Greptile on #348.
Greptile's follow-up is right that no stderr pattern can prove a render
completed. `raise ... from None` would drop the chaining marker the previous
commit leaned on, and in general the absence of evidence of an earlier failure
is not evidence of its absence. Copier itself has no `from None` or
`__suppress_context__` anywhere in 9.17.1, so the specific mechanism cannot fire
today -- but the objection stands on principle, and the design that answers it
is simpler than the matcher it replaces.

So stop accepting non-zero exits at all. The race is intermittent -- the same
dirty tree renders fine on the next attempt -- so a re-render is the actual
remedy, and stderr matching now only decides whether to spend one. Three
attempts against a hit rate around one render in ten; lose three in a row and
the test fails with copier's stderr, which is the safe direction.

The matcher stays tight to keep pointless retries rare, but it is no longer
load-bearing: a false positive costs one render and then fails anyway, a false
negative fails immediately, and neither can hide a partial render. Renamed to
`_is_retryable_cleanup_race` to stop implying it certifies success.

Verified a genuine copier failure (a missing required answer) aborts on attempt
1/3 with copier's full traceback, no retry. Fast suite on a dirty tree: 208
passed.

Reported by Greptile on #348.
@ichoosetoaccept
ichoosetoaccept force-pushed the no-blanket-chmod-scripts branch from 2dbd7aa to 15c02ed Compare August 6, 2026 06:52
@ichoosetoaccept
ichoosetoaccept force-pushed the tolerate-clone-cleanup-race branch from 7d05f01 to 138e14a Compare August 6, 2026 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant