diff --git a/docs/IMPLEMENTATION_SPEC.md b/docs/IMPLEMENTATION_SPEC.md index f324449..4ea736b 100644 --- a/docs/IMPLEMENTATION_SPEC.md +++ b/docs/IMPLEMENTATION_SPEC.md @@ -494,12 +494,16 @@ Status обязан показывать наличие таблицы/chain/п | `/run/awg2-harness/firewall.lock` | 10 с | `apply_firewall` (host + anti-torrent) | | `/run/awg2-harness/sysctl_tuning.lock` | 30 с | `ensure_sysctl_tuning` | +Реализация — advisory-блокировка ядра (`flock` на Linux) через `std::fs::File`, а не PID-файл. + Поведение: - ожидание захвата — с интервалом 100 мс до таймаута, затем ошибка `lock busy timeout`; -- в файл записывается PID владельца; -- если процесс-владелец мёртв (`/proc/` отсутствует) или содержимое файла не парсится как PID, блокировка считается устаревшей и перезахватывается; -- освобождение автоматическое при выходе из области видимости, включая пути с ошибкой. +- PID пишется в файл **только как диагностика**; ни одно решение на его основе не принимается; +- освобождение выполняет ядро при завершении процесса — любым способом, включая `kill -9`. Понятия «устаревшая блокировка» не существует; +- файл блокировки никогда не удаляется: сам факт его наличия ничего не значит, значение имеет только удерживаемая на нём блокировка. + +Прежняя реализация на PID-файлах имела типичные для такого подхода дефекты: конфликт определялся по подстроке в тексте ошибки; «устаревшая» блокировка удалялась и создавалась заново двумя отдельными операциями, поэтому два процесса могли одновременно счесть её устаревшей и оба захватить; переиспользованный PID делал живую блокировку похожей на мёртвую; удаление файла в `Drop` гонялось с процессом, который уже открыл его и ждёт. ## 15. Контракт вывода status diff --git a/traffic_core/src/lock.rs b/traffic_core/src/lock.rs index b796afd..3db9bb8 100644 --- a/traffic_core/src/lock.rs +++ b/traffic_core/src/lock.rs @@ -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 { + 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 { - 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 { - let pid_raw = fs::read_to_string(path)?; - let pid = match pid_raw.trim().parse::() { - 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 { - 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"); } }