refactor(lock): use kernel file locking instead of a hand-rolled PID lock [P2]
The previous implementation had every failure mode a home-grown PID lock has: - contention was detected by matching the substring "lock already exists" in an error message, so any change to the message would have turned a busy lock into a hard error; - stale-lock recovery removed the file and created it again as two separate operations, so two processes could each decide the lock was stale and each take it; - a reused PID made a live lock look stale, since the check was only whether /proc/<pid> exists; - Drop deleted the lock file, racing with a process that had already opened it and was sleeping on the retry loop - it would then lock a file that no longer has a directory entry, and the next arrival would create a fresh one and lock that instead. Replaced with advisory kernel locking through std::fs::File::try_lock, stabilised in Rust 1.89 and backed by flock on Linux. No new dependency. The kernel releases the lock when the holder exits by any means, including kill -9, so there is no stale state to reason about and no recovery heuristic to get wrong. The PID is still written to the file, but purely as diagnostics - nothing reads it back to make a decision. The lock file is now never deleted. Its presence carries no meaning; only the lock held on it does. That removes the race the old Drop introduced. Five tests: a second acquisition of a live lock fails, timeout reports lock busy timeout, the lock is free again once the guard drops, the file survives release, and a leftover file from a previous run does not block acquisition. Behaviour note for the four call sites (traffic, firewall, sysctl_tuning, swap): timeouts are unchanged, and a process killed mid-reconcile no longer leaves anything that needs cleaning up before the next run. cargo test --workspace: 158 + 4 passed. clippy -D warnings clean.
This commit is contained in:
+104
-63
@@ -1,13 +1,34 @@
|
||||
//! Advisory locks for the reconcile paths.
|
||||
//!
|
||||
//! Backed by kernel file locking (`flock` on Linux) through `std::fs::File`, not by a
|
||||
//! PID file. The previous implementation had the failure modes that home-grown PID locks
|
||||
//! always have:
|
||||
//!
|
||||
//! - it detected contention by matching a substring of an error message;
|
||||
//! - "stale lock" recovery deleted the file and re-created it as two separate steps, so
|
||||
//! two processes could both decide a lock was stale and both take it;
|
||||
//! a PID can also be reused by an unrelated process, making a live lock look stale;
|
||||
//! - `Drop` deleted the file, which races with another process that already has it open.
|
||||
//!
|
||||
//! The kernel releases the lock when the holding process exits, however it exits, so
|
||||
//! there is no stale state to reason about. The lock file itself is never deleted: its
|
||||
//! presence means nothing, only the lock held on it does.
|
||||
|
||||
use std::{
|
||||
fs::{self, OpenOptions},
|
||||
io::Write,
|
||||
fs::{self, File, OpenOptions},
|
||||
io::{Seek, SeekFrom, Write},
|
||||
path::Path,
|
||||
thread,
|
||||
time::{Duration, Instant},
|
||||
};
|
||||
|
||||
/// Holding this guard holds the lock; dropping it releases it.
|
||||
///
|
||||
/// There is no path field: nothing needs to know where the lock lives after acquisition,
|
||||
/// because the guard never deletes or re-opens the file.
|
||||
#[derive(Debug)]
|
||||
pub struct FileLock {
|
||||
path: String,
|
||||
file: File,
|
||||
}
|
||||
|
||||
impl FileLock {
|
||||
@@ -17,94 +38,114 @@ impl FileLock {
|
||||
}
|
||||
|
||||
pub fn acquire_timeout(path: &str, timeout: Duration) -> anyhow::Result<Self> {
|
||||
let path = Path::new(path);
|
||||
if let Some(parent) = path.parent() {
|
||||
fs::create_dir_all(parent).map_err(|e| {
|
||||
anyhow::anyhow!("failed to create lock directory {}: {e}", parent.display())
|
||||
})?;
|
||||
}
|
||||
|
||||
let file = OpenOptions::new()
|
||||
.create(true)
|
||||
.read(true)
|
||||
.write(true)
|
||||
.truncate(false)
|
||||
.open(path)
|
||||
.map_err(|e| anyhow::anyhow!("failed to open lock {}: {e}", path.display()))?;
|
||||
|
||||
let deadline = Instant::now() + timeout;
|
||||
loop {
|
||||
match Self::try_acquire_once(path) {
|
||||
Ok(lock) => return Ok(lock),
|
||||
Err(e) => {
|
||||
let msg = e.to_string();
|
||||
if !msg.contains("lock already exists") {
|
||||
return Err(e);
|
||||
}
|
||||
match file.try_lock() {
|
||||
Ok(()) => break,
|
||||
Err(fs::TryLockError::WouldBlock) => {
|
||||
if Instant::now() >= deadline {
|
||||
anyhow::bail!("lock busy timeout: {path}");
|
||||
anyhow::bail!("lock busy timeout: {}", path.display());
|
||||
}
|
||||
thread::sleep(Duration::from_millis(100));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn try_acquire_once(path: &str) -> anyhow::Result<Self> {
|
||||
let p = Path::new(path);
|
||||
if let Some(parent) = p.parent() {
|
||||
fs::create_dir_all(parent)?;
|
||||
}
|
||||
match OpenOptions::new().create_new(true).write(true).open(p) {
|
||||
Ok(mut file) => {
|
||||
file.write_all(std::process::id().to_string().as_bytes())?;
|
||||
Ok(Self {
|
||||
path: path.to_string(),
|
||||
})
|
||||
}
|
||||
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {
|
||||
let stale = is_stale_lock(p)?;
|
||||
if stale {
|
||||
let _ = fs::remove_file(p);
|
||||
let mut file = OpenOptions::new().create_new(true).write(true).open(p)?;
|
||||
file.write_all(std::process::id().to_string().as_bytes())?;
|
||||
Ok(Self {
|
||||
path: path.to_string(),
|
||||
})
|
||||
} else {
|
||||
anyhow::bail!("lock already exists: {path}");
|
||||
Err(fs::TryLockError::Error(e)) => {
|
||||
return Err(anyhow::anyhow!("failed to lock {}: {e}", path.display()));
|
||||
}
|
||||
}
|
||||
Err(e) => Err(e.into()),
|
||||
}
|
||||
|
||||
let lock = Self { file };
|
||||
// Diagnostic only. Nothing reads this back to make decisions - the kernel lock is
|
||||
// the authority.
|
||||
lock.record_owner();
|
||||
Ok(lock)
|
||||
}
|
||||
|
||||
fn record_owner(&self) {
|
||||
let mut f = &self.file;
|
||||
let _ = f.set_len(0);
|
||||
let _ = f.seek(SeekFrom::Start(0));
|
||||
let _ = writeln!(f, "{}", std::process::id());
|
||||
let _ = f.flush();
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for FileLock {
|
||||
fn drop(&mut self) {
|
||||
let _ = fs::remove_file(&self.path);
|
||||
// Explicit for clarity; closing the handle would release it anyway. The file is
|
||||
// deliberately left in place - deleting it would race with a process that has
|
||||
// already opened it and is waiting on the lock.
|
||||
let _ = self.file.unlock();
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn is_stale_lock(path: &Path) -> anyhow::Result<bool> {
|
||||
let pid_raw = fs::read_to_string(path)?;
|
||||
let pid = match pid_raw.trim().parse::<u32>() {
|
||||
Ok(v) => v,
|
||||
Err(_) => return Ok(true),
|
||||
};
|
||||
|
||||
let proc_path = format!("/proc/{pid}");
|
||||
Ok(!Path::new(&proc_path).exists())
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
fn is_stale_lock(_path: &Path) -> anyhow::Result<bool> {
|
||||
Ok(false)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn lock_path(name: &str) -> String {
|
||||
std::env::temp_dir()
|
||||
.join(format!("awg2_lock_{}_{}", name, std::process::id()))
|
||||
.to_string_lossy()
|
||||
.to_string()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn second_acquire_fails_for_live_lock() {
|
||||
let p = format!("/tmp/awg2_harness_lock_test_{}", std::process::id());
|
||||
let p = lock_path("live");
|
||||
let _first = FileLock::acquire(&p).expect("first lock");
|
||||
let second = FileLock::acquire(&p);
|
||||
assert!(second.is_err());
|
||||
assert!(FileLock::acquire(&p).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn acquire_timeout_times_out_for_live_lock() {
|
||||
let p = format!("/tmp/awg2_harness_lock_test_timeout_{}", std::process::id());
|
||||
let p = lock_path("timeout");
|
||||
let _first = FileLock::acquire(&p).expect("first lock");
|
||||
let second = FileLock::acquire_timeout(&p, Duration::from_millis(250));
|
||||
assert!(second.is_err());
|
||||
let err = FileLock::acquire_timeout(&p, Duration::from_millis(250)).expect_err("busy");
|
||||
assert!(err.to_string().contains("lock busy timeout"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lock_is_released_when_guard_is_dropped() {
|
||||
let p = lock_path("release");
|
||||
{
|
||||
let _first = FileLock::acquire(&p).expect("first lock");
|
||||
}
|
||||
let _second = FileLock::acquire(&p).expect("must be free again");
|
||||
}
|
||||
|
||||
/// The lock file outlives the guard on purpose: removing it would race with a waiter
|
||||
/// that already holds an open handle to it.
|
||||
#[test]
|
||||
fn lock_file_is_not_deleted_on_release() {
|
||||
let p = lock_path("persist");
|
||||
{
|
||||
let _guard = FileLock::acquire(&p).expect("lock");
|
||||
assert!(Path::new(&p).exists());
|
||||
}
|
||||
assert!(Path::new(&p).exists());
|
||||
}
|
||||
|
||||
/// A leftover file from a previous run carries no meaning; only the kernel lock does.
|
||||
#[test]
|
||||
fn existing_lock_file_does_not_block_acquisition() {
|
||||
let p = lock_path("leftover");
|
||||
fs::write(&p, "999999\n").expect("seed stale pid");
|
||||
let _guard = FileLock::acquire(&p).expect("must acquire despite leftover file");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user