Fix transaction race for file refs, fixes: #2600#2659
Open
Will Stott (willstott101) wants to merge 4 commits into
Open
Fix transaction race for file refs, fixes: #2600#2659Will Stott (willstott101) wants to merge 4 commits into
Will Stott (willstott101) wants to merge 4 commits into
Conversation
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. Co-authored-by: Claude (Anthropic) <noreply@anthropic.com>
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)`. Probe the ref path before locking so the collision is reported as a clear I/O error. The value is re-read under the lock for the race-free CAS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`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>
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".
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.
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
1 participant
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.