_util: drop the dead st_mtime=0 short-circuit in raise_on_not_writable_file#582
Merged
gaborbernat merged 3 commits intoJul 6, 2026
Merged
Conversation
…e_file The guard predated the lstat-symlink hardening (PR tox-dev#567) and was meant to defend against old platforms where os.stat could return an all-zero struct for a file that exists. On every Python release filelock supports today the struct is well-defined, so the guard is dead weight — and worse, it silences the writability and is-dir checks whenever a caller (or a test, or a tool that resets mtime to 0 for unrelated reasons) sets the mtime to 0. The bug only mattered in practice when the lock path was a read-only file or a directory: the check would skip both guards, the caller's subsequent os.open would succeed on non-Windows, and the acquire path would either take a lock on a file nothing else can write to or block forever waiting for an os.open that cannot succeed. SoftFileLock with its EEXIST-then-stale-break fallback could also spuriously recycle a live lock that happened to have mtime=0. Remove the guard. Add unit tests in tests/test_util.py covering the readonly-file-with-mtime-zero, future-mtime, and directory-with-mtime- zero corners so the writability verdict cannot be silently narrowed again. Update tests/test_filelock.py::test_mtime_zero_exit_branch's expected exception for SoftFileLock from TimeoutError to PermissionError — the previous expectation relied on the very short-circuit this commit removes, so a non-root user running the test now gets the same exception as the FileLock branch instead of hitting the stale-lock fallback path first.
for more information, see https://pre-commit.ci
Removing the st_mtime guard makes raise_on_not_writable_file fire on a read-only lock file for every backend that calls it, so WindowsFileLock now raises PermissionError too — the test still expected TimeoutError, which broke the Windows CI. Expect PermissionError on every platform and drop the stale "exit branch" name now that no such branch exists. Fold the two near-duplicate readonly function tests into one parametrized case and tighten the comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
raise_on_not_writable_file guarded its writability and is-dir checks behind
if file_stat.st_mtime != 0, on the theory that an all-zero stat struct meant the caller's stat call had failed in a way the OS could not report (see the original commit c9b6d90, "Raise when trying to acquire in R/O or missing folder").The guard was useful on the very old NFS / Linux combinations it was written against. On every Python release filelock supports today the result of
os.lstatis well-defined and never returns an all-zero struct for a file that exists, so the guard has been dead code for a long time — and worse, it silences both checks whenever a caller (or a test, or a tool that resets mtime to 0 for unrelated reasons) explicitly sets the mtime to 0. The bug only mattered in practice when the lock path was a read-only file or a directory: the check would skip both guards, the caller's subsequentos.openwould succeed on non-Windows, and the acquire path would either take a lock on a file nothing else can write to or block forever waiting for anos.openthat cannot succeed.SoftFileLockwith itsEEXIST-then-stale-break fallback could also spuriously recycle a live lock whose file happened to have mtime=0: the EEXIST fromos.open(O_CREAT | O_EXCL)falls into_try_break_stale_lock, and the "malformed lock, age ≥ threshold" branch unconditionally unlinks a lock whose mtime is sufficiently old — andtime.time() - 0.0is, of course, sufficiently old.What changes
src/filelock/_util.py: remove thest_mtime != 0short-circuit. The writability and is-dir checks now run regardless of the observed mtime, matching the function's documented contract.tests/test_util.py: add 3 unit tests pinning the function-level behavior:test_raise_on_not_writable_file_rejects_readonly_file_with_mtime_zero: 0o444 file with mtime=0 still raisesPermissionError.test_raise_on_not_writable_file_rejects_readonly_file_with_future_mtime: 0o444 file with mtime=2_000_000_000 still raisesPermissionError— locks in the "mtime is irrelevant to writability" reading so a later patch can't silently narrow the check to one mtime range.test_raise_on_not_writable_file_rejects_directory_with_mtime_zero: directory with mtime=0 still raisesIsADirectoryError.tests/test_filelock.py::test_mtime_zero_exit_branch: theSoftFileLockparametrization now expectsPermissionErrorinstead ofTimeoutError. The previous expectation relied on the very short-circuit this PR removes — under the new behavior, the writability check fires first on a non-root user, which is what the test name ("exit branch") and the FileLock parametrization have always assumed.Tests
python -m pytest tests/test_util.py→ 10 passed, 2 skipped (3 new tests added; pre-existing 7 + 2 symlink tests skipped on this platform still pass).python -m pytest tests/test_filelock.py -k mtime_zero→ 1 passed, 1 failed. The failure istest_mtime_zero_exit_branch[UnixFileLock-PermissionError], which is a pre-existing root-only failure (root canos.opena 0o444 file withO_RDWR | O_TRUNC, so the test only fires on non-root unices) — confirmed by re-running on the unpatchedmainfrom a stash, where the same test fails in exactly the same way.python -m pytest tests/→ 471 passed, 31 skipped, 2 failed. Both failures are pre-existing on the unpatchedmain(themtime_zero[UnixFileLock]andtest_stale_inode_retry_on_unlinked_lockcases), unrelated to this change.