Cleanup stale lock files in the .lsp-locks directory #6816
Conversation
| assert!(!lock_path.exists(), "Lock file should be removed"); | ||
|
|
||
| // Cleanup after test | ||
| test_lock.release().expect("Failed to release test lock"); |
There was a problem hiding this comment.
nit: not a biggie, maybe we should use tempfile to ensure file/dir cleanup (automatically done when var goes out of scope - even if a test fails mid-way)
| /// Cleans up all stale lock files in the .lsp-locks directory | ||
| /// Returns a vector of paths that were cleaned up |
There was a problem hiding this comment.
nit:
| /// Cleans up all stale lock files in the .lsp-locks directory | |
| /// Returns a vector of paths that were cleaned up | |
| /// Cleans up stale lock files in the .lsp-locks directory. | |
| /// A lock file is considered stale if: | |
| /// - It has a .lock extension | |
| /// - Either its contents cannot be parsed as a valid PID | |
| /// - Or the PID it contains is no longer active | |
| /// | |
| /// Returns paths of the lock files that were removed. |
| if let Some(ext) = path.extension().and_then(|ext| ext.to_str()) { | ||
| if ext == "lock" { | ||
| if let Ok(mut file) = File::open(&path) { | ||
| let mut contents = String::new(); | ||
| if file.read_to_string(&mut contents).is_ok() { | ||
| if let Ok(pid) = contents.trim().parse::<usize>() { | ||
| if !Self::is_pid_active(pid) { | ||
| remove_file(&path)?; | ||
| cleaned_paths.push(path); | ||
| } | ||
| } else { | ||
| remove_file(&path)?; | ||
| cleaned_paths.push(path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: reduced nesting
| if let Some(ext) = path.extension().and_then(|ext| ext.to_str()) { | |
| if ext == "lock" { | |
| if let Ok(mut file) = File::open(&path) { | |
| let mut contents = String::new(); | |
| if file.read_to_string(&mut contents).is_ok() { | |
| if let Ok(pid) = contents.trim().parse::<usize>() { | |
| if !Self::is_pid_active(pid) { | |
| remove_file(&path)?; | |
| cleaned_paths.push(path); | |
| } | |
| } else { | |
| remove_file(&path)?; | |
| cleaned_paths.push(path); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // Skip non-lock files | |
| if path.extension() | |
| .and_then(|ext| ext.to_str()) | |
| .map_or(false, |ext| ext == "lock") | |
| { | |
| let contents = match fs::read_to_string(&path) { | |
| Ok(content) => content, | |
| Err(_) => continue, | |
| }; | |
| let should_remove = contents.trim() | |
| .parse::<usize>() | |
| .map_or(true, |pid| !Self::is_pid_active(pid)); | |
| if should_remove { | |
| remove_file(&path)?; | |
| cleaned_paths.push(path); | |
| } | |
| } |
| let mut contents = String::new(); | ||
| if file.read_to_string(&mut contents).is_ok() { | ||
| if let Ok(pid) = contents.trim().parse::<usize>() { | ||
| if !Self::is_pid_active(pid) { |
There was a problem hiding this comment.
I've stopped using PID files because I've been burned by this before - the call to is_pid_active() can still return true on servers with long uptimes that are burning through processes, causing PIDs to wrap. On some systems the PID space is as small as 32768 but on my beast it's:
> cat /proc/sys/kernel/pid_max
4194304
Instead, I now use advisory locking (the following is code from forc-telemetry), which also has the nice advantage in that it's shorter code as you don't need to read the file:
self.logfile_lock = match Flock::lock(logfile, FlockArg::LockExclusiveNonblock) {
Ok(logfile_lock) => Some(logfile_lock),
Err((_, Errno::EWOULDBLOCK)) => {
// Silently exit as another Collector is already running
exit(0);
}
}
| remove_file(&path)?; | ||
| cleaned_paths.push(path); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I don't think the else should do clean up as there's a race condition - the other process could be in the middle of writing it's PID into the lockfile itself 😅
|
This made me check the forc locks and same thing happens there too, without any active forc process I have more than 50 locks in edit: opened #6818 |
Description
I noticed there were quite a few old stale lock files in the
.lsp-locksdirectory. We now check that the PID is active for each file in this directory, if it's not active then it's removed.Checklist
Breaking*orNew Featurelabels where relevant.