diff --git a/docs/audits/2026-08-12-PR-562-lifecycle-cleanup-reaudit.md b/docs/audits/2026-08-12-PR-562-lifecycle-cleanup-reaudit.md new file mode 100644 index 00000000..b73a7249 --- /dev/null +++ b/docs/audits/2026-08-12-PR-562-lifecycle-cleanup-reaudit.md @@ -0,0 +1,41 @@ +# PR #562 lifecycle cleanup independent re-audit + +- Date: 2026-08-12 +- Audited head: `052f03df8d98579f1121022ee6455aefa2346fb0` +- Scope: `ui/src-tauri/src/lifecycle.rs` +- Auditor: independent temporary audit agent +- Verdict: **PASS** + +## Findings + +No blocking or non-blocking findings. + +## Independent assessment + +- The cleanup retry is bounded: after the initial attempt it retries at most five + times, sleeping 10 ms between attempts. Persistent contention returns without + modifying the owner record. +- The implementation acquires the path's exclusive OS lock before reading and + comparing `instance_id`. It clears the file only for the matching owner. +- `LockGuard::clear` truncates and syncs the locked file in place. It does not + unlink the path, so the inode-based lock contract remains intact. +- Commit `052f03d` resolves the test scheduling race reported by CodeRabbit. The + contention callback signals the child and waits for it to exit before the + retry loop attempts to acquire the lock again. +- `origin/main` did not independently modify `lifecycle.rs` after the PR merge + base; the combined tree has no conflict affecting this verdict. + +## Verification + +- `cargo test -p airp-ui lifecycle::tests:: --locked`: 18 passed. +- The contention recovery test run 30 consecutive times: 30 passed. +- `git diff --check origin/main...HEAD`: passed. +- GitHub Rust lint, Rust test, Rust doc, UI/WebUI, production topology, portable + Windows, and CodeRabbit checks passed for the audited head. + +## Residual boundary + +The approximately 50 ms retry budget intentionally covers short-lived cleanup +contention. Longer contention preserves the owner record for later startup +recovery. This is the bounded behavior requested by issue #478, not an audit +finding for this PR. diff --git a/ui/src-tauri/src/lifecycle.rs b/ui/src-tauri/src/lifecycle.rs index 913ab7c7..609c52f8 100644 --- a/ui/src-tauri/src/lifecycle.rs +++ b/ui/src-tauri/src/lifecycle.rs @@ -184,22 +184,45 @@ fn read_lock_from_file(file: &mut File) -> Option { serde_json::from_str(&raw).ok() } +// Five retries × 10 ms bounds shutdown contention waits to about 50 ms. +const CLEANUP_LOCK_MAX_RETRIES: usize = 5; +const CLEANUP_LOCK_RETRY_DELAY: std::time::Duration = std::time::Duration::from_millis(10); + /// 仅当锁文件归属给定 instance_id 时清空(退出清理防误删他人锁)。 /// /// The same OS lock used during startup is acquired before checking ownership; /// this closes the read→remove race. The inode is retained so a POSIX peer /// cannot unlink-and-recreate the path while the guard is held. pub fn remove_lock_if_owned(path: &Path, instance_id: &str) { + remove_lock_if_owned_with_retry(path, instance_id, || {}); +} + +fn remove_lock_if_owned_with_retry(path: &Path, instance_id: &str, mut on_would_block: F) +where + F: FnMut(), +{ if !path.exists() { return; } - let mut guard = match acquire_lock(path) { - Ok(guard) => guard, - Err(error) if error.kind() == io::ErrorKind::NotFound => return, - Err(error) if error.kind() == io::ErrorKind::WouldBlock => return, - Err(error) => { - tracing::warn!(%error, "failed to acquire engine instance lock for cleanup"); - return; + + let mut retries = 0; + let mut guard = loop { + match acquire_lock(path) { + Ok(guard) => break guard, + Err(error) if error.kind() == io::ErrorKind::NotFound => return, + Err(error) + if error.kind() == io::ErrorKind::WouldBlock + && retries < CLEANUP_LOCK_MAX_RETRIES => + { + retries += 1; + on_would_block(); + std::thread::sleep(CLEANUP_LOCK_RETRY_DELAY); + } + Err(error) if error.kind() == io::ErrorKind::WouldBlock => return, + Err(error) => { + tracing::warn!(%error, "failed to acquire engine instance lock for cleanup"); + return; + } } }; let owned = guard @@ -535,6 +558,78 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + #[test] + fn owned_cleanup_retries_after_transient_lock_contention() { + let path = std::env::var_os("AIRP_LIFECYCLE_CLEANUP_CHILD_LOCK_PATH") + .map(std::path::PathBuf::from); + if let Some(path) = path { + let ready = std::env::var_os("AIRP_LIFECYCLE_CLEANUP_CHILD_READY") + .map(std::path::PathBuf::from) + .expect("cleanup child ready path"); + let release = std::env::var_os("AIRP_LIFECYCLE_CLEANUP_CHILD_RELEASE") + .map(std::path::PathBuf::from) + .expect("cleanup child release path"); + let _guard = acquire_lock(&path).expect("cleanup child must acquire lifecycle lock"); + std::fs::write(ready, b"ready").unwrap(); + for _ in 0..5_000 { + if release.exists() { + return; + } + std::thread::sleep(std::time::Duration::from_millis(1)); + } + panic!("parent did not release cleanup child lifecycle lock within timeout"); + } + + let dir = std::env::temp_dir().join(format!("airp-lifecycle-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join(LOCK_FILE_NAME); + let ready = dir.join("cleanup-child.ready"); + let release = dir.join("cleanup-child.release"); + let owner = lock(100, 200, 8000); + let mut guard = acquire_lock(&path).unwrap(); + guard.write_lock(&owner).unwrap(); + drop(guard); + + let test_name = "lifecycle::tests::owned_cleanup_retries_after_transient_lock_contention"; + let mut child = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test_name, "--nocapture"]) + .env("AIRP_LIFECYCLE_CLEANUP_CHILD_LOCK_PATH", &path) + .env("AIRP_LIFECYCLE_CLEANUP_CHILD_READY", &ready) + .env("AIRP_LIFECYCLE_CLEANUP_CHILD_RELEASE", &release) + .spawn() + .unwrap(); + + let mut ready_seen = false; + for _ in 0..250 { + if ready.exists() { + ready_seen = true; + break; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + if !ready_seen { + let _ = child.kill(); + let _ = child.wait(); + panic!("cleanup child did not publish lock-held readiness within timeout"); + } + + let mut saw_contention = false; + let mut child_status = None; + remove_lock_if_owned_with_retry(&path, &owner.instance_id, || { + saw_contention = true; + std::fs::write(&release, b"release").unwrap(); + child_status = Some(child.wait().unwrap()); + }); + assert!( + saw_contention, + "cleanup must observe the held lock before retrying" + ); + assert!(child_status.unwrap().success()); + assert!(read_lock(&path).is_none()); + + std::fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn lock_guard_keeps_owner_record_on_same_inode() { let dir = std::env::temp_dir().join(format!("airp-lifecycle-{}", uuid::Uuid::new_v4()));