fix(util): create lock files in one exclusive step - #2267
Merged
Byron merged 2 commits intoOct 2, 2026
Merged
Conversation
`LockFile._obtain_lock_or_raise()` tested for the lock with `osp.isfile()` and then created it with `open(lock_file, "w")`. Nothing keeps another holder out between the two calls, so several can pass the test and all of them set `_owns_lock`. Racing eight holders on one lock file leaves seven believing they own it, which removes the mutual exclusion that `GitConfigParser` in write mode and `RefLog.append_entry()` rely on. `osp.isfile()` also resolves symbolic links. A dangling symlink planted at `<file>.lock` therefore reports no lock, and the following `open()` resolves it and creates the target, outside the repository. Create the lock with `os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)` instead. That is the single-step exclusive create `gitdb`'s `LockedFD.open()` and Git's own `lock_file()` already use, and `O_EXCL` fails with `EEXIST` on a symbolic link rather than resolving it, so both problems close together. The creation mode matches `LockedFD`; nothing reads a lock file's contents, and breaking a stale lock needs write permission on the containing directory rather than on the file. `FileExistsError` is translated back into the existing "did already exist" `OSError`, so `BlockingLockFile`'s retry loop and the existing `test_lock_file` and `test_blocking_lock_file` cases are unaffected. A directory at the lock path is still reported through the generic `OSError` branch. This covers acquiring the lock only; writing the locked file stays the caller's concern, as before. Adds `test_lock_file_does_not_follow_a_symlink` and `test_lock_file_is_obtained_by_a_single_holder`. Both fail on the previous code (`1 != 7`, and the symlink target gets created) and pass here. Validation: full `pytest` suite green on Python 3.11 on macOS, plus `ruff check`, `ruff format`, `mypy` and `basedpyright --warnings` clean.
Member
|
Great point - let me take care of the Windows issue. |
Byron
force-pushed
the
lock-file-exclusive-create
branch
from
October 2, 2026 06:03
f272dd3 to
47a2225
Compare
<!-- Byron --> Took brief look only. Hope this code soon won't be present anymore. <!-- agent --> Windows follows dangling symlinks even when `os.open` uses `O_CREAT | O_EXCL`, so acquiring a lock could create its symlink target and incorrectly report ownership. The `_winapi.CreateFile` workaround also uses the ANSI API on Python 3.8 through 3.10, causing failures in Unicode directories or creating locks under mangled filenames. Use `CreateFileW` with `CREATE_NEW` and `FILE_FLAG_OPEN_REPARSE_POINT` to create the lock atomically while rejecting existing links. Declare the `ctypes` argument and return types explicitly so Unicode paths and native handle sizes are preserved. Close the handle before recording ownership, propagate Windows errors, and reject embedded NULs before the native API can truncate a path. Preserve exclusive `os.open` creation on POSIX. Expand the lock tests to cover Unicode filenames and directories, including non-BMP characters, and verify that the requested lock path is actually created and removed. Check NUL rejection, preserve both existing and missing symlink targets, and explicitly release the concurrent test's acquired locks. Reproduced the CI failure in `test_clone_from_with_path_contains_unicode` on Windows/Python 3.8.10 before the fix. The affected utility, clone, and configuration modules pass on Python 3.8.10 and 3.13.14: 169 passed, 41 skipped, and 2 expected failures on each version. The new Unicode and NUL regressions also failed before their respective fixes. `ruff check`, `ruff format --check`, `mypy --python-version=3.13`, and `basedpyright --warnings` pass. Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6 <codex@openai.com>
Byron
force-pushed
the
lock-file-exclusive-create
branch
from
October 2, 2026 06:37
47a2225 to
97a5468
Compare
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
3 participants
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.
Racing eight holders on one
LockFile, then planting a dangling symlink at the lock path:_obtain_lock_or_raisetests withosp.isfileand then creates withopen(lock_file, "w"). Nothing keeps another holder out between the two calls, so several pass the test and all of them set_owns_lock, which is the mutual exclusionGitConfigParserin write mode andRefLog.append_entryrely on.osp.isfilealso resolves symlinks, so a dangling symlink at<file>.lockreports no lock and theopen()follows it.os.open(..., O_WRONLY | O_CREAT | O_EXCL)does both in one step and is whatgitdb'sLockedFD.openalready does;O_EXCLfails withEEXISTon a symlink instead of resolving it.FileExistsErroris translated back to the existing "did already exist"OSError, so callers andBlockingLockFile's retry loop see the same behavior, and the creation mode matchesLockedFD's0600.The two new tests fail on the current code (
1 != 7, and the symlink target gets created) and pass here. Full suite green on Python 3.11 on macOS;ruff,mypyandbasedpyright --warningsclean.I'm an AI agent contributing through this account; this change was prepared with AI assistance.