Fix transaction race for file refs, fixes: #2600#2659
Merged
Sebastian Thiel (Byron) merged 5 commits intoJun 23, 2026
Merged
Conversation
Will Stott (willstott101)
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0771253af2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
Member
Tasks
|
Sebastian Thiel (Byron)
force-pushed
the
fix-ref-transaction-toctou
branch
from
241d392 to
0c569a0
Compare
`create::Iter` already distinguishes a path component that exists as a non-directory from one that is a directory (via `dir.is_dir()`), but it forwarded the raw `AlreadyExists` error in the former case. That made a path collision indistinguishable from genuine lock contention once it reached `gix-lock` (which maps `AlreadyExists` to `PermanentlyLocked`). Surface the non-directory case as `NotADirectory` so callers can tell the two apart from the failing operation itself, without a separate probe. In gix-ref this lets lock-acquisition I/O errors surface as `Error::Io` (reserving `Error::LockAcquire` for actual contention), which restores the historical `Io(NotADirectory)` for path-prefix collisions in the `Change::Update` branch and removes the pre-lock probe added previously. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Read the existing ref value after acquiring the lock rather than before, so the PreviousValue check compares against a value that cannot be changed concurrently. Without this, two transactions can both read the old value, serialize through the lock, and both pass the CAS check — silently losing one update. The TOCTOU fix moved the existing-ref read under the lock, which for the Change::Update branch is now after lock acquisition. A path-prefix collision (creating `a/b` while `a` is a ref file) therefore surfaced via the lock's create_dir_all as `AlreadyExists`, reported as the misleading `LockAcquire(PermanentlyLocked)` rather than the historical `Io(NotADirectory)`. Thanks to the `gix-fs` fix, lock-acquisition I/O errors surface as `Error::Io` (reserving `Error::LockAcquire` for actual contention), which restores the historical `Io(NotADirectory)` for path-prefix collisions in the `Change::Update` branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move the prohibit_windows_device_names validation ahead of lock acquisition in lock_ref_and_apply_change(). Previously the check ran inside ref_contents(), which is called by read_existing_ref() after the .lock file was already created. For a ref such as refs/heads/CON the lock path CON.lock is itself a reserved Windows device name, so prepare() would fail during lock acquisition (or open the device) rather than returning the configured validation error. Extract the device-name check from ref_contents() into a new check_windows_device_name() helper on the store and call it at the top of lock_ref_and_apply_change(), before either the Delete or Update branch tries to acquire a lock. ref_contents() still delegates to the same helper for callers that read refs outside the transaction path.
Add `gix free remote refs` to perform an upload-pack handshake, discover the remote reference advertisement, and print the refs without negotiating or receiving a pack. The command can also write the advertised refs into a standalone ref store: ```sh gix free remote refs --refs-directory out-refs <url> ``` Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
Sebastian Thiel (Byron)
force-pushed
the
fix-ref-transaction-toctou
branch
from
0c569a0 to
6767b30
Compare
Member
|
Thanks so much! All I could do here is to clean up the commits a bit, and make just a minor refactor. Overall, I believe this is sound, and much better than it was before. After seeing all this complexity just to deal with the filesystem, and Windows, I really can't wait to just swap in RefTables and be done. |
Member
|
There is one more note: the global lock is a performance optimization which works for The global lock optimisation I will probably tackle in a follow-up. |
Contributor
Author
|
Thanks for your tidy up and merge!
|
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.
When two ref transactions update the same ref concurrently (e.g. two pushes doing main:
A→B), both could read the old valueA, serialize through the lock, and both pass theMustExistAndMatchcheck— silently losing one update. The fix reads the existing value after acquiring the lock, so the comparison is against a value that can't change underneath us. See: #2600
While rebasing this onto the current tree, one
gix-reftest started failing. Moving the read under the lock meant a path-prefix collision (creatinga/bwhileais a ref file) was now hit by thelock's
create_dir_allfirst, which reportsAlreadyExistsand surfaced as a misleading"permanently locked"error instead of the previousNotADirectory.My first pass restored the old behaviour with a small ref_contents probe before locking. That worked, but it added a filesystem read on every update just to produce a nicer error on a rare one — so I
went looking for a better spot and found that
gix-fs's directory-creation loop already distinguishes a non-directory in the path (it does theis_dir()check) and was simply throwing that informationaway by forwarding the raw
AlreadyExists. Reporting it asNotADirectorythere letsgix-refsurface lock-acquisition I/O errors asError::Io(keepingError::LockAcquirefor genuine contention), whichfixes the error for every
gix-lockcaller and lets me drop the probe entirely, keeping the transaction code simpler.