fix(runtime): Add cleanup for orphaned temp directories#96
Conversation
Addresses issue #81 by adding a new `moat system clean-temp` command that scans for and removes orphaned temporary directories in /tmp. Moat creates temporary directories for AWS credentials, Claude config, and Codex config. These are normally cleaned up when a run completes, but accumulate if moat crashes or is killed. Changes: - Add internal/system/tempclean.go with directory scanning logic - Add cmd/moat/cli/system_cleantemp.go command implementation - Update CLI reference documentation - Support --dry-run, --force, and --min-age flags - Only clean directories older than specified age (default: 1h) The command scans for these patterns: - agentops-aws-* (AWS credential helpers) - moat-claude-staging-* (Claude configuration) - moat-codex-staging-* (Codex configuration)
📦 Previous Review (superseded by newer review)Pull Request Review: Add cleanup for orphaned temp directoriesThis PR adds a useful maintenance command to clean up temporary directories that may accumulate after crashes. The implementation is solid overall with good user experience. Here are my findings: ✅ Strengths
🔍 Potential Issues1. TOCTOU Race Condition (Medium Severity)// internal/system/tempclean.go:46-54
info, err := os.Stat(match)
if err != nil {
continue
}
// Skip if directory was modified recently (still in use)
if info.ModTime().After(cutoff) {
continue
}Issue: There's a time-of-check-time-of-use (TOCTOU) vulnerability between:
Scenario: A new moat run could start and create a temp directory between the scan and deletion, potentially removing an active directory if it happens to reuse a previously scanned path. Recommendation: Re-verify directory age immediately before deletion: func CleanOrphanedTempDirs(dirs []OrphanedTempDir, minAge time.Duration) error {
var errs []string
cutoff := time.Now().Add(-minAge)
for _, dir := range dirs {
// Re-check age before deletion to avoid race condition
if info, err := os.Stat(dir.Path); err == nil {
if info.ModTime().After(cutoff) {
// Directory was modified since scan - skip it
continue
}
}
if err := os.RemoveAll(dir.Path); err != nil {
errs = append(errs, fmt.Sprintf("%s: %v", dir.Path, err))
}
}
// ... rest of function
}2. Missing Test Coverage (Medium Severity)Issue: No unit tests for the new functionality. The PR description mentions "CLI tests pass" but there are no test files for:
Recommendation: Add tests for:
Example test structure: func TestFindOrphanedTempDirs(t *testing.T) {
// Create temp directories with known ages
// Verify correct filtering by minAge
// Verify proper handling of permission errors
}3. Silent Error Handling in dirSize (Low Severity)// internal/system/tempclean.go:90-101
func dirSize(path string) (int64, error) {
var size int64
err := filepath.Walk(path, func(_ string, info os.FileInfo, err error) error {
if err != nil {
return nil // Skip files we can't access
}
// ...
})
return size, err
}Issue: Silently skipping inaccessible files means reported sizes may be inaccurate without user awareness. Recommendation: Consider logging or counting skipped files, especially since this is a cleanup tool where accuracy matters for user decision-making. 4. No Active Run Cross-Check (Low-Medium Severity)Issue: The help text claims directories are "not associated with active runs" but there's no actual verification. The implementation only checks modification time, which could give false positives if a long-running container hasn't modified its temp directory recently. Recommendation: Consider adding a check against active containers via the container runtime (similar to how 🎯 Code Quality
📊 Performance Considerations
🔒 Security Considerations
📝 Minor Suggestions
✅ RecommendationAPPROVE with changes requested The feature is valuable and well-implemented. The TOCTOU race condition should be fixed before merging, and test coverage should be added. The other issues are lower priority but would improve robustness. Priority fixes:
Great work on the UX and documentation! 🎉 |
Address review feedback from PR #96: 1. Fix TOCTOU race condition - Re-verify directory age before deletion - Skip directories modified since initial scan - Prevents deleting newly created temp dirs 2. Add comprehensive unit tests - TestFindOrphanedTempDirs with various age scenarios - TestCleanOrphanedTempDirs for successful cleanup - TestCleanOrphanedTempDirs_SkipsRecentlyModified for race protection - TestFormatSize edge cases - TestDirSize directory size calculation - TestPluralSuffix helper function 3. Update documentation - Remove inaccurate "not associated with active runs" claim - Documentation now accurately reflects what the command checks All tests pass and the TOCTOU race condition is now prevented.
📦 Previous Review (superseded by newer review)Pull Request Review: Add cleanup for orphaned temp directoriesThis PR adds a useful maintenance command to clean up temporary directories that may accumulate when moat crashes. The implementation is well-structured with good test coverage and clear documentation. Below are my findings: Code Quality ✅Strengths:
Suggestions:
// Re-check age before deletion to avoid TOCTOU race condition
// A new moat run could have started between scan and deletion
if info, err := os.Stat(dir.Path); err == nil {
if info.ModTime().After(cutoff) {
skipped = append(skipped, dir.Path)
continue
}
}The comment is there, but it could be more prominent as this is a critical security feature.
Consider extracting this to a shared utility or keeping only one implementation. Potential Issues 🔍
Performance Considerations ⚡
Currently, this is fine since these are temporary directories, not user workspaces. Security Concerns 🔒Excellent security posture:
One consideration:
Test Coverage ✅Excellent test coverage:
Missing tests:
Consider adding a test for permission errors: func TestCleanOrphanedTempDirs_PermissionError(t *testing.T) {
tmpDir := t.TempDir()
testDir := filepath.Join(tmpDir, "test-readonly")
if err := os.Mkdir(testDir, 0000); err != nil {
t.Fatal(err)
}
defer os.Chmod(testDir, 0755) // Clean up
orphaned := []OrphanedTempDir{{Path: testDir}}
err := CleanOrphanedTempDirs(orphaned, 1*time.Hour)
if err == nil {
t.Error("expected error cleaning readonly directory")
}
}Documentation 📚Strengths:
Minor suggestions:
Git Conventions 🔧Issue with commit format: The PR description states "do not include Co-Authored-By lines for Claude" per CLAUDE.md, but the PR body includes:
This is fine for PR descriptions, but ensure the actual commit messages follow the Conventional Commits format without Co-Authored-By lines. The commit title
Suggested: Overall Assessment ⭐This is a solid PR that adds valuable functionality. The implementation is clean, well-tested, and follows project conventions. The security considerations (TOCTOU protection, confirmation prompts, dry-run mode) demonstrate thoughtful design. Recommended actions before merge:
Rating: 8.5/10 - High-quality implementation with comprehensive testing and good security practices. The minor suggestions are refinements rather than blockers. |
Pull Request Review: Add cleanup for orphaned temp directoriesThis PR successfully addresses issue #81 by implementing a manual cleanup command for orphaned temporary directories. The implementation is well-structured with good test coverage and clear documentation. Code Quality ✅Strengths:
Minor suggestions:
Potential Issues 🔍No critical issues found. The implementation correctly handles the TOCTOU race condition by re-verifying directory age before deletion. Observation: The // dirSize calculates the total size of a directory recursively.
// Inaccessible files are silently skipped.
func dirSize(path string) int64 {
var size int64
filepath.Walk(path, func(_ string, info os.FileInfo, err error) error {
if err == nil && !info.IsDir() {
size += info.Size()
}
return nil
})
return size
}Performance Considerations ⚡Good: The implementation is efficient for the expected use case (small number of temp directories). The re-verification before deletion adds minimal overhead while preventing dangerous race conditions. Note:
Security Concerns 🔒Excellent security posture:
Consideration: The patterns are hardcoded in
Test Coverage 🧪Excellent test coverage:
All key scenarios are tested. The TOCTOU test is particularly valuable as it validates the security-critical re-verification logic. Documentation 📚Strong documentation:
Matches CLAUDE.md style guide: Objective tone, factual claims, practical examples first. Alignment with Issue #81 ✅The PR fully addresses the issue requirements:
Note: The issue mentioned considering "finalizers or background cleanup" as alternatives. This PR implements the manual cleanup approach, which is more conservative and gives users control. Background cleanup could be a future enhancement if needed. Overall Assessment ⭐Recommend: APPROVE with minor suggestions This is a well-crafted PR that solves a real problem (temp directory accumulation on crashes) with a safe, tested implementation. The security considerations (TOCTOU protection, confirmation prompts, pattern restrictions) demonstrate thoughtful design. The only suggestions are minor code clarity improvements. The implementation follows Go best practices, aligns with the project's style guide, and includes comprehensive tests and documentation. |
1 participant
Fixes #81
Summary
moat system clean-tempcommand to clean up orphaned temporary directories/tmpfor moat temp directories older than specified age (default: 1h)--dry-run,--force, and--min-ageflagsProblem
Moat creates temporary directories for AWS credential helpers, Claude config, and Codex config. These are normally cleaned up when a run completes, but accumulate if moat crashes.
Solution
Added a new
moat system clean-tempcommand that scans for orphaned directories and safely removes them after confirmation.Testing
🤖 Generated with Claude Code