feat(bf-2w7): add comprehensive cleanup on all exit paths

- Add cleanup_performed flag to HookInstaller for idempotent cleanup
- Add Drop implementation to HookInstaller for automatic cleanup
- Enhance cleanup() to explicitly remove both FIFO and temp directory
- Ensure temp dirs are cleaned up on normal exit, error, timeout, signals, and panic
- cleanup_orphans() already called at startup to sweep stale temp dirs

Co-Authored-By: Claude <noreply@anthropic.com>

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<AtomicBool>` 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

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<AtomicBool>,
 }
 
 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);
     }
 }