fix(fslock): delete lock file on Release#375
Merged
Merged
Conversation
Release previously left the lock file on disk to avoid a TOCTOU where one process unlinks the file while another has just opened it and is about to flock — the two would end up holding locks on different inodes at the same path. Delete safely by: (unix) unlinking before close so new openers can't see our inode, plus an inode recheck after flock that reopens if the path no longer points at our fd; (windows) Close then os.Remove, which Go opens without FILE_SHARE_DELETE so Remove fails harmlessly while any handle is open.
…ows Release error swallow - Unix: stale-inode retry loop now checks the deadline before continuing and returns a proper timeout error when the budget is exhausted, so Acquire cannot exceed its requested timeout. - Windows: Release only swallows os.Remove failures that indicate another handle is still open (ERROR_SHARING_VIOLATION / ERROR_ACCESS_DENIED) or the file is already gone. Other filesystem errors now propagate. Addresses roborev review 1845 (M+L).
ERROR_ACCESS_DENIED is not a reliable signal that another handle is keeping the file open — Windows also returns it for read-only attributes, ACL changes, or the path being replaced with a directory. Only ERROR_SHARING_VIOLATION unambiguously indicates the deferred-cleanup case, so narrow isBenignRemoveErr to just that and ErrNotExist. Other filesystem errors now propagate from Release. Addresses roborev review 1846.
…omment - Update Release doc comment: only ERROR_SHARING_VIOLATION (and ErrNotExist) are swallowed; ERROR_ACCESS_DENIED now propagates. - Add fslock_windows_test.go covering sharing-violation (benign), not-exist (benign), access-denied (propagated), and generic (propagated) cases to lock in the distinction going forward. Addresses roborev review 1847.
amoeba
approved these changes
May 5, 2026
Member
amoeba
left a comment
amoeba
left a comment
There was a problem hiding this comment.
Tested manually and it looks good.
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
2 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.
Summary
Releasenow deletes the lock file instead of leaving it on disk.Acquirethat has opened the path but not yet taken the lock, leaving two processes with locks on different inodes at the same path.flock— if the path no longer points at our fd's inode, we release, close, and reopen via a retry loop.Closethenos.Remove. Go opens files withoutFILE_SHARE_DELETE, soRemoveonly succeeds once no other process holds a handle — we swallow the error if cleanup can't happen this round.Test plan
go test ./internal/fslock/ -race -count=1 -v— all pass, including two new tests:TestReleaseRemovesFileverifies the file is gone afterRelease(would have failed on old code).TestConcurrentAcquireIsExclusivespawns 8 goroutines that acquire/hold/release and asserts the max concurrent holder count is 1, exercising the delete/reopen path.GOOS=windows go build ./internal/fslock/compiles cleanly.go test ./... -shortpasses.