Avoid file lock warning for in-process contention#1406
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1406 +/- ##
=======================================
Coverage 90.38% 90.39%
=======================================
Files 82 82
Lines 16208 16262 +54
=======================================
+ Hits 14650 14700 +50
- Misses 1558 1562 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📦 Cargo Bloat ComparisonBinary size change: +0.00% (22.5 MiB → 22.5 MiB) Expand for cargo-bloat outputHead Branch ResultsBase Branch Results |
There was a problem hiding this comment.
Pull request overview
This pull request improves the file lock contention detection logic to avoid showing spurious warnings when lock contention occurs within the same process. It adds reference counting to track which locks are held by the current process and suppresses the "Another prek process may still be running" warning for in-process contention.
Changes:
- Added reference counting mechanism (
IN_PROCESS_LOCK_HELD_COUNTS) to track locks held by the current process - Modified
LockedFileto store the lock path and maintain reference counts on acquire/release - Updated warning logic to check if lock is held in-process before showing cross-process warning
- Refactored test infrastructure with new test-only globals (
LOCK_WARNING_PATHS,FORCE_CROSS_PROCESS_LOCK_WARNING_FOR) - Improved user-facing message in reporter from "Initializing hooks..." to "Cloning repos..."
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/prek/src/fs.rs | Implements in-process lock tracking with reference counting and updates warning logic; includes refactored tests |
| crates/prek/src/cli/reporter.rs | Updates progress message from "Initializing hooks..." to "Cloning repos..." for better clarity |
| } | ||
|
|
||
| impl LockedFile { | ||
| /// Inner implementation for [`LockedFile::acquire_blocking`] and [`LockedFile::acquire`]. |
There was a problem hiding this comment.
The comment references LockedFile::acquire_blocking, but this method does not exist in the codebase. The lock_file_blocking method is now a private helper that only returns Result<fs_err::File, std::io::Error>, not a LockedFile. Update the comment to accurately reflect the current implementation, or remove the reference to acquire_blocking if it's no longer part of the public API.
| /// Inner implementation for [`LockedFile::acquire_blocking`] and [`LockedFile::acquire`]. | |
| /// Helper to acquire an exclusive lock on a file, blocking if necessary. |
| if let Err(err) = self.file.file().unlock() { | ||
| error!( | ||
| "Failed to unlock {}; program may be stuck: {}", | ||
| self.0.path().display(), | ||
| self.file.path().display(), | ||
| err | ||
| ); | ||
| } else { | ||
| trace!(path = %self.0.path().display(), "Released lock"); | ||
| let mut held = IN_PROCESS_LOCK_HELD_COUNTS.lock().unwrap(); | ||
| if let Some(count) = held.get_mut(&self.path) { | ||
| *count = count.saturating_sub(1); | ||
| if *count == 0 { | ||
| held.remove(&self.path); | ||
| } | ||
| } | ||
| trace!(path = %self.file.path().display(), "Released lock"); | ||
| } |
There was a problem hiding this comment.
The reference count is only decremented when the unlock operation succeeds (inside the else block). If unlock() fails, the count remains incremented, which will cause future lock contention on the same path to incorrectly believe the lock is still held by this process, suppressing warnings that should be shown.
The count should be decremented regardless of whether the unlock succeeds or fails, since the LockedFile is being dropped and will no longer hold the lock from the application's perspective. Move the count decrement logic (lines 184-190) outside of the if-else statement so it always executes.
Follow up for #1353