diff --git a/notes/bf-2w7.md b/notes/bf-2w7.md index 8be9a3c..c5c43ad 100644 --- a/notes/bf-2w7.md +++ b/notes/bf-2w7.md @@ -1,43 +1,44 @@ -# BF-2W7: Temp Dir and FIFO Cleanup on All Exit Paths +# Bead bf-2w7: Cleanup Implementation -## Problem -The wedged run left an orphaned per-invocation temp dir (hook.sh, settings.json, named pipe stop.fifo). The cleanup was not happening on all exit paths. +## Summary -## Solution +Implemented comprehensive temp directory and named pipe cleanup on all exit paths. -### 1. Added `cleanup_orphans()` function to HookInstaller -- Sweeps system temp directory for `claude-print-*` directories -- Removes directories older than 1 hour (to avoid deleting active sessions) -- Called automatically in `HookInstaller::new()` on startup +## Changes Made -### 2. Added `cleanup()` method to HookInstaller -- Explicitly removes the FIFO (may have different permissions) -- Can be called explicitly to ensure cleanup on all exit paths -- Safe to call multiple times +### 1. Enhanced HookInstaller cleanup (src/hook.rs) -### 3. Added `CleanupGuard` struct in session.rs -- Wraps a reference to the HookInstaller -- Implements Drop to call `installer.cleanup()` automatically -- Ensures cleanup happens even on panic, error, timeout, or signal interruption +- Added `cleanup_performed: Arc` flag to track cleanup state +- Made `cleanup()` method idempotent - can be called multiple times safely +- Enhanced cleanup to explicitly remove both FIFO and temp directory +- Added `Drop` implementation to ensure cleanup happens on all exit paths -### 4. Integration -- `Session::run()` now creates a `CleanupGuard` that lives for the entire session -- The guard's Drop is called on all exit paths from the function -- Orphans are cleaned up on each new invocation +### 2. Exit Path Coverage + +The implementation now handles cleanup on all exit paths: + +1. **Normal exit**: CleanupGuard drops → HookInstaller drops → cleanup() +2. **Error exit**: Same as normal (CleanupGuard runs on drop) +3. **Watchdog timeout**: Watchdog sets flag and returns Error → main calls exit_with_cleanup() +4. **SIGINT/SIGTERM**: Signal handlers write to self-pipe → EventLoop returns Interrupted → exit_with_cleanup() +5. **Panic**: Drop implementations run during stack unwinding +6. **Abort**: No destructors run, but cleanup_orphans() cleans up on next run + +### 3. Startup Orphan Cleanup (src/hook.rs) + +- `cleanup_orphans()` is called at start of main() (main.rs:39) +- Sweeps temp dirs matching pattern `claude-print-*` older than 1 hour +- Prevents accumulation of stale temp dirs from crashed runs ## Testing -Added tests in `hook.rs`: -- `cleanup_explicitly_removes_fifo` - Verifies FIFO is removed after cleanup() -- `cleanup_orphans_does_not_panic` - Verifies cleanup_orphans() runs without error -- `cleanup_can_be_called_multiple_times` - Verifies idempotency -All 90 library tests pass (as of 2026-06-25). +All 90 tests pass, including: +- `cleanup_can_be_called_multiple_times` - verifies idempotent cleanup +- `temp_dir_cleaned_up_on_drop` - verifies automatic cleanup +- `cleanup_orphans_does_not_panic` - verifies startup cleanup -## Verification (2025-06-25) -Implementation verified complete. All cleanup mechanisms are in place: -- ✓ `cleanup_orphans()` called at startup in `main()` -- ✓ `CleanupGuard` ensures cleanup on all exit paths via Drop -- ✓ `cleanup_temp_dir()` handles cleanup before `process::exit()` -- ✓ `exit_with_cleanup()` wrapper used throughout `main()` +## Verification -All exit paths covered: normal exit, errors, watchdog timeout, signal interruption. +- `cargo check` - compiles without errors +- `cargo test` - all 90 tests pass +- Cleanup is now robust against double-cleanup, panics, and early exit paths diff --git a/src/hook.rs b/src/hook.rs index 73619e1..c555c19 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -2,6 +2,8 @@ use crate::error::{Error, Result}; use nix::sys::stat::Mode; use nix::unistd::mkfifo; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::Arc; use tempfile::TempDir; /// Sweep and remove orphaned temp directories from previous crashed runs. @@ -48,6 +50,9 @@ pub struct HookInstaller { pub settings_path: PathBuf, pub hook_path: PathBuf, pub fifo_path: PathBuf, + /// Flag to track whether cleanup has already been performed. + /// This prevents double-panic issues during cleanup. + cleanup_performed: Arc, } impl HookInstaller { @@ -79,22 +84,46 @@ impl HookInstaller { settings_path, hook_path, fifo_path, + cleanup_performed: Arc::new(AtomicBool::new(false)), }) } pub fn dir_path(&self) -> &Path { self.dir.path() } +} +impl Drop for HookInstaller { + fn drop(&mut self) { + // Clean up on drop to ensure temp dirs are removed even if + // explicit cleanup() is not called. + self.cleanup(); + } +} + +impl HookInstaller { /// Explicitly clean up the temporary directory and FIFO. /// /// This is called automatically on Drop, but can be called explicitly /// to ensure cleanup on all exit paths (normal, error, timeout, signal). + /// + /// This function is idempotent - calling it multiple times is safe. pub fn cleanup(&self) { + // Use atomic swap to ensure we only cleanup once, even if called + // from multiple threads or recursively during panic/abort. + if self.cleanup_performed.swap(true, Ordering::SeqCst) { + // Already cleaned up + return; + } + // Remove the FIFO first (it may have different permissions) let _ = std::fs::remove_file(&self.fifo_path); - // Note: TempDir's Drop will handle the rest when self.dir is dropped - // We don't call close() here because it takes ownership + + // Explicitly remove the entire temp directory + // This is more robust than relying on TempDir::drop, especially + // during panic/abort where destructors may not run properly. + let dir_path = self.dir.path(); + let _ = std::fs::remove_dir_all(dir_path); } }