Fix atomic writes replacing destinations after command failures - #1918
Fix atomic writes replacing destinations after command failures#1918wterrr wants to merge 1 commit into
Conversation
kramlipi
left a comment
kramlipi
left a comment
There was a problem hiding this comment.
Automated code review by kramlipi code-agent.
Review notes from kramlipi code-agent (@kramlipi_code_review_bot) — grounded scanners + critic, not a rubber stamp.
Free for open-source / public repos. Ready-made GitHub Actions: code-review CI setup · Product: www.kramlipi.com · Docs: https://kramlipi.github.io/
| return # pragma: no cover | ||
| self._f.close() | ||
| os.replace(self._tmp_filename, self._real_filename) | ||
| if delete: |
There was a problem hiding this comment.
Should fix · reliability · typer/_click/_compat.py:419 · Potential resource leak on os.replace failure
What: The close method uses os.replace without ensuring the temporary file is cleaned up if the replacement fails (e.g., cross-device link error or permission issue).
Why it matters: Leaving orphaned temporary files in the filesystem can lead to disk space exhaustion or security issues if sensitive data is left in a predictable location.
Do this: Wrap os.replace in a try/finally block to ensure os.unlink(self._tmp_filename) is called if the replacement fails.
Suggestion:
| if delete: | |
| try: | |
| os.replace(self._tmp_filename, self._real_filename) | |
| finally: | |
| if os.path.exists(self._tmp_filename): | |
| os.unlink(self._tmp_filename) |
Callers that may care:
typer/testing.py:33→__init__—super().__init__()typer/testing.py:65→__init__—super().__init__(buffer, **kwargs)tests/test_prepare_release.py:34→__init__—new_content = update_version_file(content, "0.26.3", Path("typer/__init__.py"))tests/test_prepare_release.py:43→__init__—update_version_file(content, "0.26.2", Path("typer/__init__.py"))pyproject.toml:92→__init__—version = { source = "file", path = "typer/__init__.py" }
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| import typer |
There was a problem hiding this comment.
Worth a look · consistency · tests/test_atomic_file.py:6 · PR title/diff inconsistency
What: PR title emphasizes topics that do not appear in changed paths or added code (destinations, failures, replacing, writes).
Why it matters: Align the message or the diff. (scanner:commit_consistency)
Do this: Either fix it in this PR, or reply here explaining why the current behavior is intentional.
Evidence: title='Fix atomic writes replacing destinations after command failures'; missing_in_diff=['destinations', 'failures', 'replacing', 'writes']
|
|
||
| if ctx is not None: | ||
| ctx.call_on_close(lf.close_intelligently) | ||
| if self.atomic: |
There was a problem hiding this comment.
Worth a look · design · typer/_click/types.py:542 · No behavior risk: api_signature
What: No behavior risk: api_signature
Why it matters: The changes to File.convert and _AtomicFile logic are internal implementation details of how resources are managed within the Click context, and do not alter the public API signature of File or ParamType.
Do this: Either fix it in this PR, or reply here explaining why the current behavior is intentional.
Evidence: typer/_click/types.py:542
Callers that may care:
typer/testing.py:33→__init__—super().__init__()typer/testing.py:65→__init__—super().__init__(buffer, **kwargs)tests/test_prepare_release.py:34→__init__—new_content = update_version_file(content, "0.26.3", Path("typer/__init__.py"))tests/test_prepare_release.py:43→__init__—update_version_file(content, "0.26.2", Path("typer/__init__.py"))pyproject.toml:92→__init__—version = { source = "file", path = "typer/__init__.py" }
Skipped findings (line not in PR diff or over max_comments)
Review notes from kramlipi code-agent (@kramlipi_code_review_bot) — grounded scanners + critic, not a rubber stamp. Free for open-source / public repos. Ready-made GitHub Actions: code-review CI setup · Product: www.kramlipi.com · Docs: https://kramlipi.github.io/ |
Review notes3 note(s) — 0 must-fix · 1 should-fix · 2 worth a look · 1 from deterministic scanners Themes: reliability (1), consistency (1), design (1) Start here:
Review notes from kramlipi code-agent (@kramlipi_code_review_bot) — grounded scanners + critic, not a rubber stamp. Free for open-source / public repos. Ready-made GitHub Actions: code-review CI setup · Product: www.kramlipi.com · Docs: https://kramlipi.github.io/ |
3 participants
Summary
Problem
File parameters configured with
atomic=Truereplace the destination evenwhen the command callback raises an exception after writing partial content.
This defeats the purpose of atomic output and can replace an existing valid
file with incomplete data.
The issue affects both
lazy=Trueandlazy=False.Fix
Atomic files are now registered with
Context.with_resource()so theircontext managers receive the active exception.
LazyFileforwards the exception information to its underlying atomicstream, and
_AtomicFile.close(delete=True)removes the temporary fileinstead of replacing the destination.
Non-atomic file cleanup behavior is unchanged.
Tests
Regression coverage verifies both lazy modes when:
Validation results:
Related upstream report: pallets/click#3221