Skip to content

Propagate atomic write failures and clean temporary files - #194

Open
Gonghan-Princess wants to merge 2 commits into
fabiocaccamo:mainfrom
Gonghan-Princess:codex/fix-atomic-write-errors
Open

Gonghan-Princess wants to merge 2 commits into
fabiocaccamo:mainfrom
Gonghan-Princess:codex/fix-atomic-write-errors

Conversation

@Gonghan-Princess

Copy link
Copy Markdown
Contributor

Describe your changes

Atomic writes can currently report success even when the write never reaches the destination: _write_file_atomic catches every FileNotFoundError, including a real failure from os.replace. Its comment assumes the temporary file deletes itself on context exit, but the file is opened with delete=False and closed before replacement.

This also leaves temporary files behind if writing, flushing, or syncing fails before temp_path is assigned.

  • Remove the obsolete FileNotFoundError handler so callers receive the actual failure.
  • Save the temporary path immediately after creation, allowing the existing finally cleanup to handle early failures.
  • Add regression tests for replacement and fsync failures (including exception identity), a real encoding/write failure, unchanged destination content and permissions, and temporary-file cleanup.
  • Add an Unreleased changelog note. README usage and signatures are unchanged, so no README edit is needed.

The successful close-before-replace sequence, permission inheritance, and append behavior are unchanged. This branch starts from upstream main, independently of the archive-fix PR.

Related issue

No separate issue. Reproduced locally by patching fsutil.io.os.replace to raise FileNotFoundError during fsutil.write_file(existing_path, "replacement", atomic=True): before this change the call returned None and retained the old content; after this change it raises the original exception and retains the old content without temporary files.

Validation

Windows, Python 3.12.10:

  • TDD regression run before the fix: 4 failed, 1 passed. The failures demonstrate swallowed exceptions and leaked temporary files.
  • Atomic tests after the fix: 9 passed, 1 skipped (existing Windows permission-test skip).
  • python -X utf8 -m pytest tests --cov=fsutil --cov-report=term-missing --cov-fail-under=90: 170 passed, 3 skipped; 98.74% total coverage, 100% for io.py.
  • python -X utf8 -m pre_commit run --all-files --show-diff-on-failure --verbose: all configured hooks passed.
  • python -X utf8 -m mypy --install-types --non-interactive: passed.
  • git diff --check: passed.

Cross-platform CI has not been run locally; the three skipped tests are existing Windows-only skips.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added tests for the proposed changes.
  • I have run the tests and there are no errors.

AI assistance and provenance

AI assistance was used to investigate the bug, implement the minimal change, write regression tests, run checks, and draft this description. No third-party code, assets, or datasets were introduced; the new code and tests are submitted under the project's existing license.

fabiocaccamo

This comment was marked as outdated.

@codecov

codecov Bot commented Oct 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.86%. Comparing base (7ed66a8) to head (a339829).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #194      +/-   ##
==========================================
+ Coverage   98.48%   98.86%   +0.37%     
==========================================
  Files          13       13              
  Lines         794      792       -2     
==========================================
+ Hits          782      783       +1     
+ Misses         12        9       -3     
Flag Coverage Δ
unittests 98.86% <100.00%> (+0.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fabiocaccamo

Copy link
Copy Markdown
Owner

@Gonghan-Princess thanks for the PR. Before going further, could you please explain what concrete issue led you to this fix? Did you encounter this behavior in a real application, and if so, could you provide the environment and steps to reproduce it?

I'm asking because _write_file_atomic() has some intentionally tricky cross-platform behavior, especially on Windows. We have had real-world Windows Server issues in the past that were not reproducible in CI, so I want to make sure we understand the actual failure case before changing this logic.

In particular, the existing FileNotFoundError handling is intentional: on Windows, this exception can occur as part of the temporary file cleanup after the file has already been atomically replaced, so it can actually indicate a successful write.

The new tests assume that a FileNotFoundError in this flow should always be propagated, which may not be correct cross-platform.

Could you please explain the original problem and how you reproduced it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants