google / google/glazier

file_util.Move() deletes the destination before renaming, leaving a window where the config file does not exist

Open Beginner friendly
#800 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
1.3k
Forks
96
Avg merge
3d 15h
Merged PRs (30d)
4

Description

`file_util.Move()` deletes the destination before renaming onto it, which turns the config writer's replace step into two operations with a window between them:

```python
# glazier/lib/file_util.py:96-112
"""Python's os.rename doesn't support overwrite on Windows."""
try:
Remove(dst)
os.rename(src, dst)
except OSError as e:
raise FileMoveError(src, dst) from e
```

The docstring's reasoning is correct. Measured on CPython 3.13.13, Windows 11:

```python
os.rename(src, dst) # dst exists
# FileExistsError: [WinError 183] Cannot create a file when that file already exists
```

But `os.replace()`, added in 3.3, overwrites atomically on both platforms:

```python
os.replace(src, dst) # same setup
# succeeds; dst content is now src's, src is gone
```

The window matters most at `config/files.py:112`, the "Replace the original with the tmp" step of `Dump()`. That is the write-to-temp-then-swap pattern, whose whole purpose is that the live file is never absent. As written, `Remove(dst)` deletes the live config first, so an interruption between the two calls leaves no config at all rather than the previous version. `config/files.py:82` has the same shape when rotating a `.bak`.

Interruption is not hypothetical here, since Glazier restarts machines as part of a build.

`os.replace(src, dst)` is a drop-in for both lines and removes the window. Worth checking whether `Remove()` was also carrying a permissions side effect before swapping it.

I have no Linux or macOS machine, so the cross-platform half is read from the `os.replace` contract rather than measured.

Contributor guide

Open the contributing guide

Research direction

Start in glazier/lib/file_util.py:96-112 and inspect the callers at config/files.py:82 and :112. Check whether Remove() provides any permissions side effect before evaluating the replacement behavior on Windows and other supported platforms. Done means both config replacement paths avoid a window where the destination is absent while preserving the intended move semantics.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
operating-systems
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.