phasespace-labs / phasespace-labs/palinode
write_memory_file: overwriting a read-only target on Windows strands the temp file and replaces the original error
- Dominant language
- Python
- Stars
- 39
- Forks
- 42
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 37
Description
When `write_memory_file` overwrites an existing target that is read-only, `os.replace` raises `PermissionError`. The original file survives intact — that part is correct. But on native Windows **the temporary file is left behind**, and the exception the caller sees is not the one that stopped the write.
**Observed on two hosts.** Both native Windows runs on #169 recorded it: @kevin-lozada-santos on Python 3.11.15 and @Kaap10 on 3.12.7 — *"one `.tmp` file remains pending cleanup … original contents survive."*
**Probable mechanism** — read from the code, to be confirmed by your regression rather than taken on trust. Before writing, the function copies the **target's** mode onto the temp file (`mode = os.stat(file_path).st_mode & 0o777`, then `fchmod` or, on Windows, `os.chmod(tmp_path, mode)`). A read-only target therefore produces a read-only temp file. When `os.replace` then fails, the cleanup block calls `os.unlink(tmp_path)` and catches only `FileNotFoundError` — but on Windows, unlinking a file with the read-only attribute raises `PermissionError`. That escapes the `except`, **replaces the original exception**, and the temp file stays on disk. On POSIX the same code is fine, because unlink is governed by the directory's permissions rather than the file's.
**What is wanted**, in the scope you proposed on #169:
- When replacing a read-only destination fails, **preserve the destination's contents and attributes** — already true; keep it true.
- **Remove only the temporary file created by that write attempt.** On Windows that likely means clearing the read-only attribute on the temp file before unlinking it.
- **Keep the original failure meaningful to the caller.** The exception that propagates should be the one raised by `os.replace`. If cleanup itself fails, that is worth a log line — not a replacement exception.
**Please keep the fix inside the failure path.** Deferring the mode copy until after `os.replace` succeeds would trade this bug for a different one: a write that has already replaced the target and then fails to restore its permissions, which is harder to reason about than a stranded temp file. The cleanup block is the right place.
**Tests:** a native Windows regression that **fails on the current implementation** — a read-only target, an attempted overwrite, then assert that no `.tmp` remains in the directory and that **the original exception survives**. Assert on identity rather than on message text: `os.replace` sets both `filename` (the temp source) and `filename2` (the destination), so a correct error legitimately names both, and an assertion that it names one "rather than" the other would be wrong. What matters is that the error the caller catches is the one `os.replace` raised, not a second one from cleanup. Please also confirm the normal writable-overwrite path still leaves no temp file and still writes byte-for-byte — both #169 receipts verified that by hand.
**Scope:** `palinode/core/git_tools.py`'s `write_memory_file` and its tests. Nothing else in that module.
**Reserved for @kevin-lozada-santos**, who found it and proposed the scope above on #169. It is not open for another claim. Comment here when you are ready and I will sort the assignment out.
Contributor guide
Research direction
Start in palinode/core/git_tools.py at write_memory_file and read the existing tests for that function. Run the regression on native Windows with a read-only target, then verify the original os.replace exception survives, only the temporary file is removed, and writable overwrites still preserve bytes and leave no temporary file.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- operating-systems, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100