Skip to content

Commit 04dfeae

Browse files
deseratclaude
andcommitted
[EPIC-3-2] feat(sync): add stale lock detection to file locking mechanism
- What: Enhanced FileLock with stale lock detection and automatic cleanup - Why: Prevents deadlocks from abandoned locks when processes crash (Issue #13) - How: Added is_stale() and force_unlock() methods with 5-minute threshold Implementation: - Added is_stale() private method to detect locks older than 5 minutes - Added force_unlock() private method to remove stale lock files - Enhanced acquire() to automatically detect and clean stale locks before timeout - Locks are now recovered automatically without manual intervention Testing: - Added test_stale_lock_cleanup() using filetime crate - Total test suite: 6 tests (90% → 100% of acceptance criteria) - Coverage target: 80%+ (sync-service standard) Dependencies: - Added filetime = "0.2" to dev-dependencies for timestamp testing - All existing dependencies (fs2, anyhow, tempfile) already present Files modified: - sync-service/src/vault/lock.rs (~40 lines added): - Lines 224-237: is_stale() method - Lines 246-252: force_unlock() method - Lines 167-171: Stale lock check in acquire() - Lines 369-398: test_stale_lock_cleanup() test - sync-service/Cargo.toml (2 lines added): - Added [dev-dependencies] section - Added filetime = "0.2" for testing Acceptance criteria addressed: - [x] FileLock with RAII pattern (existing) - [x] acquire() with timeout (existing) - [x] release() for cleanup (existing) - [x] Drop implementation (existing) - [x] Stale lock detection (NEW - 5-minute threshold) - [x] Automatic cleanup of abandoned locks (NEW) - [x] Lock files in .noosphere/locks/ (existing) - [x] fs2, anyhow, tempfile dependencies (existing) - [x] filetime dev-dependency (NEW) - [x] 6+ comprehensive tests (5 existing + 1 new) - [x] Integration-ready for vault writer/parser Design decisions: - 5-minute stale threshold: Balances quick recovery vs false positives - Private methods: is_stale() and force_unlock() are implementation details - Automatic in acquire(): Transparent to consumers, no API changes - SystemTime for age calculation: Standard Rust approach for file timestamps Related to EPIC-3 (Vault Structure and File Utilities) #13 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 4d0ca57 commit 04dfeae

2 files changed

Lines changed: 84 additions & 0 deletions

File tree

sync-service/Cargo.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,7 @@ chrono = { version = "0.4", features = ["serde"] }
5656
# Temporary files (atomic writes)
5757
tempfile = "3.0"
5858
shellexpand = "3.1.1"
59+
60+
[dev-dependencies]
61+
# File timestamp manipulation for testing
62+
filetime = "0.2"

sync-service/src/vault/lock.rs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,13 @@ impl FileLock {
164164
return Ok(());
165165
}
166166
Err(_e) => {
167+
// Check if existing lock is stale and clean it up
168+
if self.is_stale()? {
169+
self.force_unlock()?;
170+
continue; // Retry immediately after cleaning stale lock
171+
}
172+
173+
// Lock is not stale, check timeout
167174
if start.elapsed()? > timeout {
168175
anyhow::bail!("Lock timeout: failed to acquire lock within {:?}", timeout);
169176
}
@@ -200,6 +207,48 @@ impl FileLock {
200207
}
201208
Ok(())
202209
}
210+
211+
/// Check if the lock file is stale (older than 5 minutes)
212+
///
213+
/// A stale lock indicates that the process holding the lock has died
214+
/// or is otherwise unable to release it properly.
215+
///
216+
/// # Returns
217+
///
218+
/// `Ok(true)` if lock file exists and is older than 5 minutes
219+
/// `Ok(false)` if lock file doesn't exist or is recent
220+
///
221+
/// # Errors
222+
///
223+
/// Returns an error if unable to read file metadata or system time
224+
fn is_stale(&self) -> Result<bool> {
225+
// If lock file doesn't exist, it's not stale
226+
if !self.lock_file.exists() {
227+
return Ok(false);
228+
}
229+
230+
// Get file metadata to check modification time
231+
let metadata = fs::metadata(&self.lock_file)?;
232+
let modified = metadata.modified()?;
233+
let age = SystemTime::now().duration_since(modified)?;
234+
235+
// Consider lock stale if older than 5 minutes
236+
Ok(age > Duration::from_secs(300))
237+
}
238+
239+
/// Force removal of a stale lock file
240+
///
241+
/// Should only be called after confirming the lock is stale via `is_stale()`.
242+
///
243+
/// # Errors
244+
///
245+
/// Returns an error if unable to remove the lock file
246+
fn force_unlock(&self) -> Result<()> {
247+
if self.lock_file.exists() {
248+
fs::remove_file(&self.lock_file)?;
249+
}
250+
Ok(())
251+
}
203252
}
204253

205254
/// Automatic lock cleanup using RAII pattern
@@ -316,4 +365,35 @@ mod tests {
316365

317366
Ok(())
318367
}
368+
369+
#[test]
370+
fn test_stale_lock_cleanup() -> Result<()> {
371+
use filetime::{set_file_mtime, FileTime};
372+
373+
let mut temp_file = NamedTempFile::new()?;
374+
write!(temp_file, "Test content")?;
375+
temp_file.flush()?;
376+
377+
let mut lock = FileLock::new(temp_file.path())?;
378+
379+
// Create a stale lock file manually (simulating abandoned lock)
380+
File::create(&lock.lock_file)?;
381+
382+
// Set modification time to 6 minutes ago (beyond 5-minute stale threshold)
383+
let six_min_ago = SystemTime::now()
384+
.checked_sub(Duration::from_secs(360))
385+
.unwrap();
386+
set_file_mtime(&lock.lock_file, FileTime::from_system_time(six_min_ago))?;
387+
388+
// Verify lock is detected as stale
389+
assert!(lock.is_stale()?);
390+
391+
// Should successfully acquire lock despite existing stale lock file
392+
lock.acquire(Duration::from_secs(1))?;
393+
394+
// Lock should now be held
395+
assert!(lock._guard.is_some());
396+
397+
Ok(())
398+
}
319399
}

0 commit comments

Comments
 (0)