feat: add periodic cleanup for expired workspaces

[CarlJi]

• Add automatic cleanup of expired workspaces every 24 hours
• Implement startPeriodicCleanup and cleanupExpiredWorkspaces methods
• Remove unused repo_manager.go file
• Improve cleanup logging and error handling

[qiniu-ci]

Code Review for PR #345: Workspace Cleanup Implementation

Todo List:

• Read and analyze the modified workspace manager code
• Review the removed repo_manager.go file
• Check code quality and best practices
• Identify potential bugs or issues
• Suggest improvements
• Evaluate architecture and design decisions
• Verify documentation consistency
• Provide comprehensive feedback

---

Overall Assessment

This PR implements a well-structured periodic cleanup system for expired workspaces. The implementation is solid and follows good practices. Here's my detailed feedback:

✅ Strengths

1. Clean Architecture: The periodic cleanup is properly integrated into the manager's lifecycle through NewManager() at internal/workspace/manager.go:51
2. Immediate + Periodic Cleanup: Smart design calling cleanup immediately on startup, then every 24 hours at internal/workspace/manager.go:332
3. Comprehensive Logging: Excellent logging throughout the cleanup process with detailed success/failure tracking at internal/workspace/manager.go:351-364
4. Proper Resource Management: Uses defer ticker.Stop() to prevent goroutine/ticker leaks at internal/workspace/manager.go:335
5. File Cleanup: Correctly removes unused repo_manager.go file that was just a migration marker

🔧 Suggestions for Improvements

1. Goroutine Lifecycle Management

Issue: The cleanup goroutine runs indefinitely with no way to stop it gracefully.

Suggestion: Consider adding a context for graceful shutdown:

func (m *Manager) startPeriodicCleanup(ctx context.Context) {
    ticker := time.NewTicker(24 * time.Hour)
    m.cleanupExpiredWorkspaces()
    go func() {
        defer ticker.Stop()
        for {
            select {
            case <-ticker.C:
                m.cleanupExpiredWorkspaces()
            case <-ctx.Done():
                log.Infof("Stopping periodic workspace cleanup")
                return
            }
        }
    }()
}

2. Error Handling Resilience

Location: internal/workspace/manager.go:354-362

Suggestion: Add panic recovery to prevent a single cleanup failure from crashing the goroutine:

defer func() {
    if r := recover(); r != nil {
        log.Errorf("Panic in workspace cleanup: %v", r)
    }
}()

3. Configurable Cleanup Interval

Issue: 24-hour interval is hardcoded.

Suggestion: Make the cleanup interval configurable through config.yaml:

workspace:
  base_dir: "./workspace"
  cleanup_after: "24h"
  cleanup_interval: "24h"  # New field

🐛 Potential Issues

1. Race Condition Risk

Location: internal/workspace/manager.go:356

Issue: Multiple cleanup operations could potentially interfere with active workspaces.

Mitigation: The existing code appears safe since it only cleans expired workspaces, but consider adding a brief check to ensure workspaces aren't actively being used.

2. Memory Usage

Location: internal/workspace/manager.go:345

Issue: For repositories with many expired workspaces, loading all at once could consume significant memory.

Suggestion: Consider batch processing:

// Process in batches of 50 to avoid memory spikes
const batchSize = 50
for len(expiredWorkspaces) > 0 {
    batch := expiredWorkspaces
    if len(batch) > batchSize {
        batch = expiredWorkspaces[:batchSize]
        expiredWorkspaces = expiredWorkspaces[batchSize:]
    } else {
        expiredWorkspaces = nil
    }
    // Process batch...
}

📚 Documentation Consistency

✅ Good: The README.md already documents the cleanup_after configuration at line 125:

workspace:
  cleanup_after: "24h"

Suggestion: Add a note about the automatic periodic cleanup feature:

# Workspace settings
workspace:
  base_dir: "./workspace"
  cleanup_after: "24h"    # Workspaces older than this will be automatically cleaned up every 24 hours

🏗️ Architecture & Design

Excellent Decisions:

• Integration with existing GetExpiredWorkspaces() and CleanupWorkspace() methods
• Separation of concerns - periodic scheduling vs actual cleanup logic
• Proper initialization in the constructor pattern

Minor Enhancement: Consider exposing cleanup statistics through a metrics/health endpoint for monitoring.

🧪 Testing Considerations

The implementation would benefit from:

1. Unit tests for the new methods
2. Integration tests to verify the 24-hour cycle (perhaps with a shorter interval for testing)
3. Goroutine leak tests to ensure proper cleanup

💡 Summary

This is a solid implementation that addresses workspace cleanup effectively. The code is well-structured, properly logged, and integrates cleanly with existing systems. The main improvements would be around graceful shutdown, configurability, and enhanced resilience.

The removal of the unused repo_manager.go file is appropriate and clean.

Recommendation: ✅ Approve with minor suggestions - the core functionality is sound and production-ready.

👉 Want me to re-run this review? Just type /review in the comments!

[codecov]

Codecov Report

❌ Patch coverage is 15.87983% with 196 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@91e8fc0). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
internal/workspace/manager.go	24.50%	107 Missing and 7 partials ⚠️
internal/workspace/git_service.go	0.00%	68 Missing ⚠️
internal/code/mcp_config.go	0.00%	13 Missing ⚠️
internal/code/claude_docker.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #345   +/-   ##
=======================================
  Coverage        ?   12.82%           
=======================================
  Files           ?       56           
  Lines           ?    11562           
  Branches        ?        0           
=======================================
  Hits            ?     1483           
  Misses          ?     9922           
  Partials        ?      157           

Flag	Coverage Δ
unittests	12.82% <15.87%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[CarlJi]

Code Review for PR #345: Workspace Cleanup Implementation

Todo List:

• Read and analyze the modified workspace manager code
• Review the removed repo_manager.go file
• Check code quality and best practices
• Identify potential bugs or issues
• Suggest improvements
• Evaluate architecture and design decisions
• Verify documentation consistency
• Provide comprehensive feedback

Overall Assessment

This PR implements a well-structured periodic cleanup system for expired workspaces. The implementation is solid and follows good practices. Here's my detailed feedback:

✅ Strengths

1. Clean Architecture: The periodic cleanup is properly integrated into the manager's lifecycle through NewManager() at internal/workspace/manager.go:51
2. Immediate + Periodic Cleanup: Smart design calling cleanup immediately on startup, then every 24 hours at internal/workspace/manager.go:332
3. Comprehensive Logging: Excellent logging throughout the cleanup process with detailed success/failure tracking at internal/workspace/manager.go:351-364
4. Proper Resource Management: Uses defer ticker.Stop() to prevent goroutine/ticker leaks at internal/workspace/manager.go:335
5. File Cleanup: Correctly removes unused repo_manager.go file that was just a migration marker

🔧 Suggestions for Improvements

1. Goroutine Lifecycle Management

Issue: The cleanup goroutine runs indefinitely with no way to stop it gracefully.

Suggestion: Consider adding a context for graceful shutdown:

func (m *Manager) startPeriodicCleanup(ctx context.Context) {
    ticker := time.NewTicker(24 * time.Hour)
    m.cleanupExpiredWorkspaces()
    go func() {
        defer ticker.Stop()
        for {
            select {
            case <-ticker.C:
                m.cleanupExpiredWorkspaces()
            case <-ctx.Done():
                log.Infof("Stopping periodic workspace cleanup")
                return
            }
        }
    }()
}

2. Error Handling Resilience

Location: internal/workspace/manager.go:354-362

Suggestion: Add panic recovery to prevent a single cleanup failure from crashing the goroutine:

defer func() {
    if r := recover(); r != nil {
        log.Errorf("Panic in workspace cleanup: %v", r)
    }
}()

3. Configurable Cleanup Interval

Issue: 24-hour interval is hardcoded.

Suggestion: Make the cleanup interval configurable through config.yaml:

workspace:
  base_dir: "./workspace"
  cleanup_after: "24h"
  cleanup_interval: "24h"  # New field

🐛 Potential Issues

1. Race Condition Risk

Location: internal/workspace/manager.go:356

Issue: Multiple cleanup operations could potentially interfere with active workspaces.

Mitigation: The existing code appears safe since it only cleans expired workspaces, but consider adding a brief check to ensure workspaces aren't actively being used.

2. Memory Usage

Location: internal/workspace/manager.go:345

Issue: For repositories with many expired workspaces, loading all at once could consume significant memory.

Suggestion: Consider batch processing:

// Process in batches of 50 to avoid memory spikes
const batchSize = 50
for len(expiredWorkspaces) > 0 {
    batch := expiredWorkspaces
    if len(batch) > batchSize {
        batch = expiredWorkspaces[:batchSize]
        expiredWorkspaces = expiredWorkspaces[batchSize:]
    } else {
        expiredWorkspaces = nil
    }
    // Process batch...
}

📚 Documentation Consistency

✅ Good: The README.md already documents the cleanup_after configuration at line 125:

workspace:
  cleanup_after: "24h"

Suggestion: Add a note about the automatic periodic cleanup feature:

# Workspace settings
workspace:
  base_dir: "./workspace"
  cleanup_after: "24h"    # Workspaces older than this will be automatically cleaned up every 24 hours

🏗️ Architecture & Design

Excellent Decisions:

• Integration with existing GetExpiredWorkspaces() and CleanupWorkspace() methods
• Separation of concerns - periodic scheduling vs actual cleanup logic
• Proper initialization in the constructor pattern

Minor Enhancement: Consider exposing cleanup statistics through a metrics/health endpoint for monitoring.

🧪 Testing Considerations

The implementation would benefit from:

1. Unit tests for the new methods
2. Integration tests to verify the 24-hour cycle (perhaps with a shorter interval for testing)
3. Goroutine leak tests to ensure proper cleanup

💡 Summary

This is a solid implementation that addresses workspace cleanup effectively. The code is well-structured, properly logged, and integrates cleanly with existing systems. The main improvements would be around graceful shutdown, configurability, and enhanced resilience.

The removal of the unused repo_manager.go file is appropriate and clean.

Recommendation: ✅ Approve with minor suggestions - the core functionality is sound and production-ready.

👉 Want me to re-run this review? Just type /review in the comments!

@qiniu-ci 帮我分析下这个报告

[CarlJi]

@qiniu-ci 帮我review下这个PR

[CarlJi]

@qiniu-ci 帮我review下这个PR

[CarlJi]

你好！我来介绍一下自己 👋

Hi @CarlJi! Great question!

🤖 Who am I?

I'm qiniu-ci - an AI assistant designed specifically to help with GitHub issues and pull requests for this codebase. Think of me as your automated code review and development assistant!

🛠️ What I can help with:

• Code Reviews - I can analyze PRs, identify bugs, suggest improvements, and check best practices
• Implementation Tasks - I can write code, fix issues, and make changes when asked
• Documentation - I can help explain code, write docs, and answer technical questions
• Repository Analysis - I can examine codebases, understand architecture, and provide insights

📋 My Role in this PR:

For PR #345 (your workspace cleanup implementation), I've been:

• Providing comprehensive code reviews
• Analyzing the codecov reports you mentioned
• Identifying potential improvements and issues
• Ready to help with any specific concerns you have

🤝 How to work with me:

Just @ mention me with what you need! For example:

• @qiniu-ci review this code
• @qiniu-ci fix the test coverage issue
• @qiniu-ci explain how this function works

🇨🇳 Language Support:

I can communicate in both English and Chinese (中文), whatever works better for you!

Is there anything specific about PR #345 you'd like me to help with? I'm here and ready to assist! 😊

[CarlJi]

@qiniu-ci 再帮我review下这个PR

[CarlJi]

@qiniu-ci hi

[CarlJi]

@qiniu-ci hi, who are you？

[CarlJi]

@qiniu-ci hi， who are you？