fix: use os.replace instead of shutil.move for renaming file

[guacs]

Pull Request Checklist

• New code has 100% test coverage
• (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
• (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

• This PR uses os.replace instead of shutil.move for renaming files to ensure atomicity.

Close Issue(s)

[guacs]

The current usage of shutil.move for renaming a file is not ensured to be atomic. It uses os.rename semantics under the hood, but the atomicity is not guaranteed across all platforms (Windows to be more specific) as per this issue.

Also, a point to note is that, if the destination file exists os.rename will raise a FileExistsError in Windows, but os.replace will silently overwrite the existing file.

[github-actions]

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2223

[Alc-Alc]

The current usage of shutil.move for renaming a file is not ensured to be atomic. It uses os.rename semantics under the hood, but the atomicity is not guaranteed across all platforms (Windows to be more specific) as per this issue.

Also, a point to note is that, if the destination file exists os.rename will raise a FileExistsError in Windows, but os.replace will silently overwrite the existing file.

Can you point me where it says this is atomic on Windows? The docs don't seem to mention that

[Goldziher]

@JacobCoffee - this is a good issue for a ruff rule. I see there is a new noqa added by this pr, so it should be brought to their attention I'd say

[provinzkraut]

@JacobCoffee - this is a good issue for a ruff rule. I see there is a new noqa added by this pr, so it should be brought to their attention I'd say

I don't think this would make a good rule really? Ruff can't know if you need the atomicity, and aside from that there are differences in behaviour of the functions that might otherwise be relevant.

[JacobCoffee]

as an aside, I would be interested in enabling all ruff rules rather than the 75 line list we have now.

Reason being is that as new ruff rules are added, we wouldn't know.. if we enabled them all and selectively disabled then we would get the newest new there is, and possibly greatly shorten our pyproject.toml

[Goldziher]

as an aside, I would be interested in enabling all ruff rules rather than the 75 line list we have now.

Reason being is that as new ruff rules are added, we wouldn't know.. if we enabled them all and selectively disabled then we would get the newest new there is, and possibly greatly shorten our pyproject.toml

why not do it?

[provinzkraut]

@guacs I second what @Alc-Alc said. I tried to confirm that os.replace is indeed (more) atomic than shutil.move but couldn't find anything. If this is indeed the case, this can be merged!

[Alc-Alc]

The current usage of shutil.move for renaming a file is not ensured to be atomic. It uses os.rename semantics under the hood, but the atomicity is not guaranteed across all platforms (Windows to be more specific) as per this issue.
Also, a point to note is that, if the destination file exists os.rename will raise a FileExistsError in Windows, but os.replace will silently overwrite the existing file.

Can you point me where it says this is atomic on Windows? The docs don't seem to mention that

If I understood what I read, internally MoveFileEx is used, which is also used by JDK at least in this version for atomic move. Also as per one of the answer here (not the entire answer)

It is very possible for MoveFile to be atomic, but it is also possible for something to go wrong and for it to fall back to a non-atomic behavior even if the underlying filesystem supports atomic rename.

Just posting this here for documentation purposes.

[guacs]

I wasn't able to find direct documentation that states that os.replace is atomic in Windows, but I think os.replace is the best attempt at an atomic rename. It's what is used by Python in its importlib for an atomic write as seen here. The following is a comment in that _write_atomic function:

We first write data to a temporary file, and then use os.replace() to perform an atomic rename.

Also, this issue is the one that changes from using os.unlink followed by os.rename to instead use os.replace.

[provinzkraut]

Thanks @guacs and @Alc-Alc for investigating this!