Skip to content

Fix database error handling in project creation #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
frankbria merged 3 commits into from
Dec 11, 2025
Merged

Fix database error handling in project creation #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
197ce43
fix: Add graceful database error handling to project creation API
claude Dec 10, 2025
18adf2b
Add comprehensive error handling for database operations in cleanup p…
frankbria Dec 11, 2025
46794cc
Sanitize database error messages to prevent information disclosure
frankbria Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 70 additions & 16 deletions codeframe/ui/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import logging
import os
import shutil
import sqlite3

from codeframe.core.models import (
ProjectStatus,
Expand Down Expand Up @@ -326,21 +327,34 @@ async def create_project(request: ProjectCreateRequest):
)

# Check for duplicate project name
existing_projects = app.state.db.list_projects()
try:
existing_projects = app.state.db.list_projects()
except sqlite3.Error as e:
logger.error(f"Database error listing projects: {str(e)}")
raise HTTPException(
status_code=500, detail="Database error occurred. Please try again later."
)

Comment thread
coderabbitai[bot] marked this conversation as resolved.
if any(p["name"] == request.name for p in existing_projects):
raise HTTPException(
status_code=409, detail=f"Project with name '{request.name}' already exists"
)

# Create project record first (to get ID)
project_id = app.state.db.create_project(
name=request.name,
description=request.description,
source_type=request.source_type.value,
source_location=request.source_location,
source_branch=request.source_branch,
workspace_path="", # Will be updated after workspace creation
)
try:
project_id = app.state.db.create_project(
name=request.name,
description=request.description,
source_type=request.source_type.value,
source_location=request.source_location,
source_branch=request.source_branch,
workspace_path="", # Will be updated after workspace creation
)
except sqlite3.Error as e:
logger.error(f"Database error creating project: {str(e)}")
raise HTTPException(
status_code=500, detail="Database error occurred. Please try again later."
)

# Create workspace
try:
Expand All @@ -352,13 +366,45 @@ async def create_project(request: ProjectCreateRequest):
)

# Update project with workspace path and git status
app.state.db.update_project(
project_id, {"workspace_path": str(workspace_path), "git_initialized": True}
)
try:
app.state.db.update_project(
project_id, {"workspace_path": str(workspace_path), "git_initialized": True}
)
except sqlite3.Error as db_error:
# Database error during update - cleanup and fail
logger.error(f"Database error updating project {project_id}: {db_error}")

# Best-effort cleanup: delete project record
try:
app.state.db.delete_project(project_id)
except sqlite3.Error as cleanup_db_error:
logger.error(f"Failed to delete project {project_id} during cleanup: {cleanup_db_error}")

# Best-effort cleanup: remove workspace directory (use actual workspace_path)
if workspace_path.exists():
try:
shutil.rmtree(workspace_path)
logger.info(f"Cleaned up workspace directory: {workspace_path}")
except (OSError, PermissionError) as cleanup_fs_error:
logger.error(f"Failed to clean up workspace {workspace_path}: {cleanup_fs_error}")

raise HTTPException(
status_code=500, detail="Database error occurred. Please try again later."
)

except HTTPException:
# Re-raise HTTPException from database error handling above
raise

except Exception as e:
# Cleanup: delete project and workspace if creation fails
app.state.db.delete_project(project_id)
logger.error(f"Workspace creation failed for project {project_id}: {e}")

# Best-effort cleanup: delete project record
try:
app.state.db.delete_project(project_id)
except sqlite3.Error as cleanup_db_error:
logger.error(f"Failed to delete project {project_id} during cleanup: {cleanup_db_error}")

# Explicitly clean up workspace directory if it exists
# (Defense in depth: WorkspaceManager has cleanup, but this ensures
Expand All @@ -368,13 +414,21 @@ async def create_project(request: ProjectCreateRequest):
try:
shutil.rmtree(workspace_path)
logger.info(f"Cleaned up orphaned workspace: {workspace_path}")
except Exception as cleanup_error:
except (OSError, PermissionError) 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)}")
raise HTTPException(
status_code=500, detail="Workspace creation failed. Please try again later."
)

# Return project details
project = app.state.db.get_project(project_id)
try:
project = app.state.db.get_project(project_id)
except sqlite3.Error as e:
logger.error(f"Database error retrieving project {project_id}: {str(e)}")
raise HTTPException(
status_code=500, detail="Database error occurred. Please try again later."
)

return ProjectResponse(
id=project["id"],
Expand Down
123 changes: 116 additions & 7 deletions tests/api/test_project_creation_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
3. REFACTOR: Clean up while keeping tests green
"""

import sqlite3
from unittest.mock import patch

import pytest


Expand Down Expand Up @@ -212,13 +215,119 @@ def test_create_project_via_api_then_get_status(self, api_client):
class TestProjectCreationErrorHandling:
"""Test error handling for project creation API."""

@pytest.mark.skip(
reason="Database close() creates ungraceful crashes, not 500 errors. This test design is flawed."
)
def test_create_project_handles_database_errors(self, api_client):
"""Test that database errors are handled gracefully (500 Internal Server Error)."""
# This test is skipped - see reason above
pass
def test_create_project_database_locked_error(self, api_client):
"""Test that database locked error returns 500 Internal Server Error."""
from codeframe.ui import server

with patch.object(
server.app.state.db,
"create_project",
side_effect=sqlite3.OperationalError("database is locked"),
):
response = api_client.post(
"/api/projects",
json={"name": "test-db-locked", "description": "Test project"},
)

assert response.status_code == 500
data = response.json()
assert "detail" in data
assert "database" in data["detail"].lower()

Comment on lines +218 to +236

@coderabbitai coderabbitai Bot Dec 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add type hints to test methods.

The test method lacks type hints for the api_client parameter. As per coding guidelines, use type hints throughout all Python code.

Apply this diff:

-    def test_create_project_database_locked_error(self, api_client):
+    def test_create_project_database_locked_error(self, api_client: TestClient) -> None:
         """Test that database locked error returns 500 Internal Server Error."""

You'll need to add this import at the top:

from starlette.testclient import TestClient

</review_comment_end>

🤖 Prompt for AI Agents 
In tests/api/test_project_creation_api.py around lines 218 to 236, the test
method test_create_project_database_locked_error lacks a type hint for the
api_client parameter; add the type annotation api_client: TestClient to the
method signature and ensure TestClient is imported at the top of the file (from
starlette.testclient import TestClient) so the hint resolves.

def test_create_project_disk_full_error(self, api_client):
"""Test that disk I/O error returns 500 Internal Server Error."""
from codeframe.ui import server

with patch.object(
server.app.state.db,
"create_project",
side_effect=sqlite3.OperationalError("disk I/O error"),
):
response = api_client.post(
"/api/projects",
json={"name": "test-disk-full", "description": "Test project"},
)

assert response.status_code == 500
data = response.json()
assert "detail" in data
assert "database" in data["detail"].lower() or "i/o" in data["detail"].lower()

Comment on lines +237 to +255

@coderabbitai coderabbitai Bot Dec 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add type hints to test methods.

Missing type hints for the api_client parameter.

Apply this diff:

-    def test_create_project_disk_full_error(self, api_client):
+    def test_create_project_disk_full_error(self, api_client: TestClient) -> None:
         """Test that disk I/O error returns 500 Internal Server Error."""

</review_comment_end>

🤖 Prompt for AI Agents 
In tests/api/test_project_creation_api.py around lines 237 to 255, the test
method test_create_project_disk_full_error is missing a type hint for the
api_client parameter; update the signature to include the correct type hint
(e.g., api_client: TestClient or the project’s fixture type) so the test
function is properly typed, and run tests to ensure the import/type is available
or add an import/forward reference for the TestClient type if needed.

def test_create_project_integrity_error(self, api_client):
"""Test that constraint violation error returns 500 Internal Server Error."""
from codeframe.ui import server

with patch.object(
server.app.state.db,
"create_project",
side_effect=sqlite3.IntegrityError("UNIQUE constraint failed"),
):
response = api_client.post(
"/api/projects",
json={"name": "test-integrity", "description": "Test project"},
)

assert response.status_code == 500
data = response.json()
assert "detail" in data
assert "database" in data["detail"].lower() or "constraint" in data["detail"].lower()

Comment on lines +256 to +274

@coderabbitai coderabbitai Bot Dec 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Clarify IntegrityError test scenario and add type hints.

This test simulates an IntegrityError during create_project, but the duplicate name check at lines 335-338 in server.py should prevent most IntegrityError cases from reaching create_project. Consider clarifying the test scenario (e.g., race condition between duplicate check and insert) or documenting that this tests edge cases where the duplicate check is bypassed.

Also missing type hints for the api_client parameter.

Apply this diff:

-    def test_create_project_integrity_error(self, api_client):
-        """Test that constraint violation error returns 500 Internal Server Error."""
+    def test_create_project_integrity_error(self, api_client: TestClient) -> None:
+        """Test that constraint violation error returns 500 Internal Server Error.
+        
+        This tests edge cases where IntegrityError occurs despite duplicate name checks,
+        such as race conditions or other constraint violations.
+        """

</review_comment_end>

🤖 Prompt for AI Agents 
In tests/api/test_project_creation_api.py around lines 256 to 274, clarify the
IntegrityError test by updating the docstring or adding an inline comment
stating this simulates an edge-case race condition where the duplicate-name
pre-check is bypassed (e.g., another insert occurs between check and insert),
and add a type hint for the api_client parameter (e.g., api_client: TestClient
or the appropriate fixture type) to the test signature so the test function is
typed.

def test_create_project_list_projects_database_error(self, api_client):
"""Test that database error during list_projects returns 500 Internal Server Error."""
from codeframe.ui import server

with patch.object(
server.app.state.db,
"list_projects",
side_effect=sqlite3.OperationalError("database is locked"),
):
response = api_client.post(
"/api/projects",
json={"name": "test-list-error", "description": "Test project"},
)

assert response.status_code == 500
data = response.json()
assert "detail" in data
assert "database" in data["detail"].lower()

Comment on lines +275 to 293

@coderabbitai coderabbitai Bot Dec 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add type hints to test methods.

Missing type hints for the api_client parameter.

Apply this diff:

-    def test_create_project_list_projects_database_error(self, api_client):
+    def test_create_project_list_projects_database_error(self, api_client: TestClient) -> None:
         """Test that database error during list_projects returns 500 Internal Server Error."""

</review_comment_end>

🤖 Prompt for AI Agents 
In tests/api/test_project_creation_api.py around lines 275 to 293, the test
function is missing a type hint for the api_client parameter; add an explicit
type annotation (for example api_client: TestClient) and import the matching
TestClient type (from fastapi.testclient import TestClient or the project's
fixture type) at the top of the file so the test signatures are fully typed and
consistent with other tests.

def test_create_project_update_project_database_error(self, api_client):
"""Test that database error during update_project returns 500 Internal Server Error."""
from codeframe.ui import server

with patch.object(
server.app.state.db,
"update_project",
side_effect=sqlite3.OperationalError("database is locked"),
):
response = api_client.post(
"/api/projects",
json={"name": "test-update-error", "description": "Test project"},
)

assert response.status_code == 500
data = response.json()
assert "detail" in data
assert "database" in data["detail"].lower()

def test_create_project_get_project_database_error(self, api_client):
"""Test that database error during get_project returns 500 Internal Server Error."""
from codeframe.ui import server

with patch.object(
server.app.state.db,
"get_project",
side_effect=sqlite3.OperationalError("database is locked"),
):
response = api_client.post(
"/api/projects",
json={"name": "test-get-error", "description": "Test project"},
)

assert response.status_code == 500
data = response.json()
assert "detail" in data
assert "database" in data["detail"].lower()

def test_create_project_with_extra_fields(self, api_client):
"""Test that extra fields in request are ignored."""
Expand Down
Loading