fix: Add explicit workspace cleanup on project creation failure

[frankbria]

Description

Fixes #7 - Improves workspace cleanup when project creation fails during workspace initialization.

Problem

When project creation fails during workspace initialization, the API endpoint only cleaned up the database record but didn't explicitly verify that the workspace directory was removed. If WorkspaceManager's internal cleanup failed or was interrupted, orphaned workspace directories could accumulate.

Solution

Added explicit workspace cleanup in the API endpoint's exception handler as a defense-in-depth measure:

• API endpoint now explicitly removes workspace directories after database cleanup
• Logs successful and failed cleanup attempts
• Ensures no orphaned directories even if WorkspaceManager's cleanup fails

Changes

• codeframe/ui/server.py:

  • Added import shutil for directory removal
  • Added explicit workspace cleanup in exception handler (lines 363-372)
  • Added logging for cleanup operations

• tests/api/test_workspace_cleanup.py (NEW):

  • test_workspace_cleanup_after_git_clone_failure: Verifies cleanup on git failure
  • test_workspace_cleanup_when_manager_cleanup_fails: Critical test simulating WorkspaceManager cleanup failure
  • test_no_cleanup_on_successful_creation: Sanity check for normal operation

Test Results

✅ 3 new tests - All passing
✅ 46 workspace tests - All passing
✅ 11 project creation API tests - All passing
✅ Total: 60 tests passing, 0 failures

TDD Approach

1. ✅ RED: Wrote failing test demonstrating orphaned workspaces
2. ✅ GREEN: Implemented fix - all tests pass
3. ✅ REFACTOR: Clean implementation following proposed solution

Implementation Details

The fix implements the proposed solution from issue #7:

except Exception as e:
    # Cleanup: delete project and workspace if creation fails
    app.state.db.delete_project(project_id)
    
    # Explicitly clean up workspace directory if it exists
    workspace_path = app.state.workspace_manager.workspace_root / str(project_id)
    if workspace_path.exists():
        try:
            shutil.rmtree(workspace_path)
            logger.info(f"Cleaned up orphaned workspace: {workspace_path}")
        except Exception as cleanup_error:
            logger.error(f"Failed to clean up workspace {workspace_path}: {cleanup_error}")
    
    raise HTTPException(status_code=500, detail=f"Workspace creation failed: {str(e)}")

Related

• Identified in PR Project Schema Refactoring - Flexible Source Types & Deployment Modes #6 code review
• Implements defense-in-depth cleanup strategy
• No breaking changes to existing functionality

Summary by CodeRabbit

• Bug Fixes

  • Improved cleanup of workspace directories when project creation fails to prevent orphaned files from accumulating on the filesystem.

• Tests

  • Added comprehensive tests for workspace cleanup behavior during project creation failures and recovery scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This change enhances workspace cleanup in the API endpoint's exception handler for failed project creation. When workspace initialization fails, the API now explicitly removes the orphaned workspace directory from the filesystem in addition to deleting the database record, using shutil.rmtree with error logging. The fix addresses cases where WorkspaceManager cleanup may fail or be interrupted.

Changes

Cohort / File(s)	Summary
API Server Enhancement codeframe/ui/server.py	Updated exception handler in project creation endpoint to compute workspace path from workspace root and project_id, check for directory existence, and remove it with shutil.rmtree, with logging for success and cleanup errors. HTTP 500 response message updated to reflect workspace creation failure.
Workspace Cleanup Test Coverage tests/api/test_workspace_cleanup.py	New test module with TestWorkspaceCleanupAPI suite containing three test cases: test_workspace_cleanup_after_git_clone_failure (verifies 500 response, no DB records, no leftover directories on git clone failure), test_workspace_cleanup_when_manager_cleanup_fails (mocks partial creation and cleanup failure, ensures API cleanup still removes orphaned directories), and test_no_cleanup_on_successful_creation (confirms successful creation preserves workspace and DB record).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• codeframe/ui/server.py: Review the cleanup logic for correct path computation, existence checks, error handling, and logging coverage
• tests/api/test_workspace_cleanup.py: Verify test cases comprehensively exercise both success and failure paths, including WorkspaceManager cleanup failure scenarios; ensure fixtures and mocks are correctly configured
• Pay attention to the interaction between database deletion and filesystem cleanup to ensure both succeed or both fail appropriately

Poem

🐰 A workspace once lost, now found and freed,
When creation stumbles, we plant the seed—
Database and disk both cleaned with care,
No orphaned paths left hanging there!
Hop hop!

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding explicit workspace cleanup on project creation failure, which is the primary focus of the PR.
Linked Issues check	✅ Passed	The code changes fully implement Issue #7 requirements: explicit workspace cleanup on project creation failure with proper error handling and logging.
Out of Scope Changes check	✅ Passed	All changes are directly related to Issue #7: workspace cleanup implementation in server.py and comprehensive test coverage in test_workspace_cleanup.py.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/issue-7-workspace-cleanup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #77: Workspace Cleanup on Project Creation Failure

Summary

✅ APPROVED - This is a well-implemented defense-in-depth fix that addresses a legitimate edge case in workspace cleanup.

---

Strengths

1. Excellent Problem Identification 🎯

• The PR correctly identifies that WorkspaceManager has cleanup logic (manager.py:70), but it uses ignore_errors=True, which could silently fail
• Defense-in-depth at the API layer ensures no orphaned directories even if lower-level cleanup fails
• Clear documentation in issue Improve workspace cleanup in project creation rollback #7 with proposed solution

2. High-Quality Tests ✅

The test suite is exemplary:

• test_workspace_cleanup_after_git_clone_failure: Tests real failure scenario (invalid git URL)
• test_workspace_cleanup_when_manager_cleanup_fails: Critical test using mocks to simulate partial workspace creation + failed cleanup
• test_no_cleanup_on_successful_creation: Sanity check ensuring normal operation isn't affected
• All tests follow proper TDD approach (RED → GREEN → REFACTOR)
• Comprehensive assertions checking both DB cleanup AND filesystem cleanup

3. Code Quality 💎

• Clean implementation with proper error handling
• Good logging (info on success, error on failure)
• Excellent inline comments explaining the defense-in-depth strategy
• No breaking changes to existing functionality

4. Proper Exception Handling

try:
    shutil.rmtree(workspace_path)
    logger.info(f"Cleaned up orphaned workspace: {workspace_path}")
except Exception as cleanup_error:
    logger.error(f"Failed to clean up workspace {workspace_path}: {cleanup_error}")

• Logs failure but doesn't re-raise (allows original error to propagate)
• This is correct behavior - cleanup failure shouldn't mask the original error

---

Minor Observations

1. Variable Name Shadowing ⚠️

In server.py:366, you assign to workspace_path:

workspace_path = app.state.workspace_manager.workspace_root / str(project_id)

This shadows the workspace_path variable used in the try block (line 347). While functionally correct (the try block's workspace_path is out of scope in the except block), it could be clearer to use a different name like orphaned_workspace_path or workspace_dir.

Impact: Low - code works correctly, just a minor clarity issue

2. Test Mock Complexity

The test_workspace_cleanup_when_manager_cleanup_fails test has a complex mock setup. While thorough, consider adding a comment explaining why you're not just using side_effect=RuntimeError(...):

# Note: We create a partial workspace before raising an error to simulate
# WorkspaceManager's cleanup failing AFTER workspace creation started

Impact: Very Low - test is correct and comprehensive

3. Consistency with WorkspaceManager

WorkspaceManager uses shutil.rmtree(workspace_path, ignore_errors=True) (line 70), while the API endpoint uses shutil.rmtree(workspace_path) without ignore_errors.

Recommendation: This is actually correct! The API layer should use strict removal (no ignore_errors) and log failures explicitly. The WorkspaceManager uses ignore_errors=True because it's already in an exception handler and wants to avoid cascading exceptions.

---

Security Considerations

✅ No security issues identified

• Path construction using workspace_root / str(project_id) is safe (project_id is an integer)
• shutil.rmtree is properly scoped to workspace directories
• No user-controlled paths in cleanup logic

---

Performance Considerations

✅ Negligible impact

• Cleanup only runs on failure path (rare)
• Path.exists() check is fast (<1ms)
• shutil.rmtree is I/O bound but only runs on failure

---

Test Coverage

✅ Excellent coverage

• 3 new tests covering all scenarios
• Tests verify both DB and filesystem cleanup
• Critical edge case (WorkspaceManager cleanup failure) is tested
• All 60 related tests passing

---

Alignment with CLAUDE.md

✅ Follows project conventions

• Matches TDD approach documented in CLAUDE.md
• Proper test organization (tests/api/test_workspace_cleanup.py)
• Clear commit message and PR description
• Logging follows existing patterns

---

Recommendations

1. Optional: Rename workspace_path variable in exception handler to avoid shadowing:

   workspace_dir = app.state.workspace_manager.workspace_root / str(project_id)
   if workspace_dir.exists():
       shutil.rmtree(workspace_dir)

2. Optional: Add a brief comment in the complex mock test explaining the setup

3. Consider: Should this cleanup also be added to any other project deletion endpoints? (Check if there are other places where projects are deleted)

---

Final Verdict

✅ APPROVED - Ready to merge

This PR demonstrates excellent software engineering practices:

• Real problem identified through code review
• Proper TDD approach with comprehensive tests
• Defense-in-depth implementation
• Clear documentation
• No breaking changes

The fix is minimal, focused, and solves the stated problem effectively. Great work! 🎉

---

Confidence: High - Reviewed full implementation, tests, and related code
Risk: Very Low - Defensive fix with no impact on happy path

[claude]

Code Review: Workspace Cleanup on Project Creation Failure

Summary

This PR successfully addresses issue #7 by adding defense-in-depth workspace cleanup to the API endpoint. The implementation is solid, well-tested, and follows the proposed solution exactly.

✅ Strengths

1. Excellent Test Coverage

• 3 comprehensive test cases covering the critical scenarios:
  • test_workspace_cleanup_after_git_clone_failure: Verifies cleanup on real git failures
  • test_workspace_cleanup_when_manager_cleanup_fails: Critical test simulating WorkspaceManager cleanup failure with mocking
  • test_no_cleanup_on_successful_creation: Sanity check for normal operation
• Tests use proper TDD approach (RED → GREEN → REFACTOR)
• Clear test documentation explaining WHY each test exists

2. Defense-in-Depth Pattern

• API endpoint now has explicit cleanup as a safety net
• WorkspaceManager retains its internal cleanup (line 70: shutil.rmtree(workspace_path, ignore_errors=True))
• Two layers of cleanup prevent orphaned workspaces even if one layer fails

3. Proper Error Handling

• Nested try-except for cleanup ensures API continues even if cleanup fails
• Logging for both success and failure cases aids debugging
• Original exception is still raised to caller with appropriate HTTP 500 status

4. Code Quality

• Clear inline comments explaining the defense-in-depth rationale
• Follows existing code style and conventions
• No breaking changes to existing functionality

---

🔍 Observations & Suggestions

1. Path Traversal Security (Low Priority)

The current implementation constructs paths as:

workspace_path = app.state.workspace_manager.workspace_root / str(project_id)

Observation: project_id is an integer from the database, so path traversal is not possible. However, for defense-in-depth:

Suggestion: Consider adding a path validation assertion:

workspace_path = app.state.workspace_manager.workspace_root / str(project_id)
# Sanity check: ensure workspace_path is actually a child of workspace_root
if not workspace_path.resolve().is_relative_to(app.state.workspace_manager.workspace_root.resolve()):
    logger.error(f"Path traversal attempt detected: {workspace_path}")
    raise HTTPException(status_code=500, detail="Invalid workspace path")

Rationale: Although not exploitable currently (project_id is integer-controlled), this adds another layer of security if the codebase evolves.

2. Duplicate Cleanup Logic (Minor)

Both WorkspaceManager.create_workspace() (line 68-70) and the API endpoint (line 366-372) now perform shutil.rmtree().

Trade-off Analysis:

• ✅ Pro: Defense-in-depth prevents orphaned directories
• ⚠️ Con: Slight code duplication

Suggestion (optional): Document this pattern in a comment or consider extracting to a shared utility if more endpoints need workspace cleanup in the future.

3. Test Mocking Complexity (Minor)

The test test_workspace_cleanup_when_manager_cleanup_fails uses a clever mock that creates partial workspace state. This is excellent for testing the fix!

Observation: The mock directly manipulates filesystem state. Consider adding a comment explaining why this approach was chosen over simpler mocking.

# Simulate workspace creation failure with incomplete cleanup
# We create the directory manually (not via WorkspaceManager) to test
# that the API endpoint can handle orphaned directories left by failed cleanup
def mock_create_with_partial_workspace(**kwargs):
    ...

4. Logging Consistency

Current logging:

logger.info(f"Cleaned up orphaned workspace: {workspace_path}")
logger.error(f"Failed to clean up workspace {workspace_path}: {cleanup_error}")

Suggestion: Consider adding a log entry at the start of cleanup attempt for easier debugging:

if workspace_path.exists():
    logger.warning(f"Orphaned workspace detected, attempting cleanup: {workspace_path}")
    try:
        shutil.rmtree(workspace_path)
        logger.info(f"Successfully cleaned up orphaned workspace: {workspace_path}")
    except Exception as cleanup_error:
        logger.error(f"Failed to clean up workspace {workspace_path}: {cleanup_error}")

This makes it easier to distinguish between:

1. No orphaned directory found (no log)
2. Cleanup attempted and succeeded (warning + info)
3. Cleanup attempted and failed (warning + error)

---

🎯 Verdict

Recommendation: ✅ APPROVE with minor suggestions

This PR:

• ✅ Solves the stated problem (issue Improve workspace cleanup in project creation rollback #7)
• ✅ Follows the proposed solution exactly
• ✅ Has excellent test coverage (3/3 critical scenarios)
• ✅ Implements defense-in-depth correctly
• ✅ Has proper error handling and logging
• ✅ No breaking changes
• ✅ No performance impact
• ✅ No security vulnerabilities introduced

The suggestions above are non-blocking and can be addressed in future PRs if desired. The current implementation is production-ready.

---

📊 Test Results Verification

According to PR description:

• ✅ 3 new tests - All passing
• ✅ 46 workspace tests - All passing
• ✅ 11 project creation API tests - All passing
• ✅ Total: 60 tests passing, 0 failures

Great job on comprehensive testing! 🎉

---

Reviewed by: Claude (Code Review Agent)
Review Date: 2025-12-05

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/api/test_workspace_cleanup.py (1)

12-51: Consider marking as integration test due to network dependency.

This test makes an actual network call to GitHub (https://github.com/nonexistent/invalid-repo-12345.git), which could cause flakiness if:

• Network is unavailable
• GitHub rate-limits the request
• DNS resolution fails

Since it's marked @pytest.mark.unit, consider either:

1. Changing to @pytest.mark.integration to reflect the external dependency
2. Mocking the git clone at the WorkspaceManager level (similar to test 2)

The test logic itself is correct and valuable for verifying the real end-to-end cleanup path.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ea2c8b and bf53491.

📒 Files selected for processing (2)

• codeframe/ui/server.py (2 hunks)
• tests/api/test_workspace_cleanup.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Use Python 3.11+ with async/await syntax, asyncio, and AsyncAnthropic for AI integrations in backend code
Lint code with ruff check . before committing; fix all style violations to maintain consistent code quality across the Python codebase
Use type hints with Python 3.11+ throughout all Python code; run type checking as part of quality gates to catch type errors before task completion

Files:

• tests/api/test_workspace_cleanup.py
• codeframe/ui/server.py

🧬 Code graph analysis (2)

tests/api/test_workspace_cleanup.py (3)

tests/api/conftest.py (1)

• api_client (42-89)

codeframe/core/config.py (1)

• get (290-302)

codeframe/workspace/manager.py (1)

• create_workspace (26-71)

codeframe/ui/server.py (6)

tests/agents/test_review_worker_agent.py (2)

• db (19-23)
• project_id (26-32)

tests/lib/test_metrics_tracker.py (1)

• db (14-34)

tests/integration/test_quality_gates_integration.py (2)

• db (24-29)
• project_id (72-78)

tests/agents/test_review_agent.py (1)

• db (19-39)

tests/planning/test_prd_generation.py (2)

• db (16-21)
• project_id (25-34)

codeframe/persistence/database.py (1)

• delete_project (1191-1199)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: Frontend Unit Tests
• GitHub Check: Backend Unit Tests
• GitHub Check: claude-review

🔇 Additional comments (4)

codeframe/ui/server.py (2)

15-15: LGTM!

The shutil import is appropriately added to support the explicit shutil.rmtree() cleanup in the exception handler.

---

359-374: Well-implemented defense-in-depth cleanup.

The explicit workspace cleanup logic correctly addresses Issue #7:

1. Path derivation matches WorkspaceManager's pattern (workspace_root / str(project_id))
2. Existence check prevents unnecessary operations
3. Try/except ensures cleanup failures don't mask the original error
4. Logging provides visibility for debugging orphaned workspace issues
5. The original exception detail is preserved in the HTTP 500 response

The project_id is an integer from the database, so path traversal is not a concern here.

tests/api/test_workspace_cleanup.py (2)

53-115: Well-designed test for the critical failure scenario.

This test correctly validates the core Issue #7 fix by:

1. Simulating a partial workspace creation (directory + file created)
2. Simulating WorkspaceManager's cleanup failure (raising without removing the directory)
3. Verifying that the API endpoint's explicit cleanup removes the orphaned workspace

The mock accurately reproduces the failure mode described in the issue. The inline patch import is acceptable, though moving it to module-level would align with typical Python conventions.

---

117-145: Good sanity check for successful path.

This test correctly validates that the cleanup logic doesn't interfere with successful project creation.

The test is properly isolated. The conftest.py includes an autouse fixture clean_database_between_tests that runs after each test, clearing all database records and the workspace directory. This ensures tests are independent regardless of execution order.