077 Audit Db Transaction Management 504ffdd3

# Database Transaction Management Audit

## Overview

This prompt guides you through auditing the MCP Gateway codebase for proper database transaction management. The goal is to identify endpoints missing explicit `db.commit(); db.close()` calls, which cause PostgreSQL connection leaks under load.

## Background: How Database Sessions Work

### The Problem

FastAPI uses dependency injection for database sessions via `Depends(get_db)`. The `get_db` generator yields a session and cleans up in a `finally` block:

```python
def get_db() -> Generator[Session, None, None]:
    db = SessionLocal()
    try:
        yield db
    finally:
        db.close()
```

**The issue**: Under high concurrency, the cleanup in `finally` may be delayed because:
1. The request completes its database query
2. The transaction stays open during response serialization (Pydantic model_dump, JSON encoding)
3. Response is sent to client
4. Only THEN does the `finally` block run

This causes connections to remain in PostgreSQL's `idle in transaction` state for 20-30+ seconds, exhausting the connection pool.

### The Solution

Explicitly commit and close the database session **immediately after database operations complete**, before any response processing:

```python
# CORRECT: Release transaction before response serialization
result = await service.do_operation(db, ...)
db.commit()
db.close()
return result

# INCORRECT: Transaction held during response serialization
return await service.do_operation(db, ...)
```

## Audit Instructions

### Step 1: Identify All Endpoints Using Database Sessions

Search for functions that have `db: Session = Depends(get_db)` in their signature:

```bash
grep -rn "db: Session = Depends(get_db)" mcpgateway/
```

### Step 2: Categorize Endpoints

For each endpoint, determine if it's:

1. **CUD (Create, Update, Delete)** - MUST have `db.commit(); db.close()`
2. **Read/List** - SHOULD have `db.commit(); db.close()` (commit ensures clean transaction state)
3. **Passthrough/Proxy** - May delegate to services that manage their own sessions

### Step 3: Verify Each Endpoint

For each endpoint, check:

1. **Does it have `db.commit()` before the return?**
2. **Does it have `db.close()` before the return?**
3. **Is the pattern applied BEFORE response serialization?**

### Correct Patterns

#### Pattern A: Simple Return
```python
async def get_item(item_id: str, db: Session = Depends(get_db)):
    service = ItemService(db)
    item = await service.get_item(item_id)
    if not item:
        raise HTTPException(status_code=404, detail="Not found")

    db.commit()
    db.close()
    return ItemResponse.from_orm(item)
```

#### Pattern B: Complex Response Building
```python
async def list_items(db: Session = Depends(get_db)):
    service = ItemService(db)
    items = await service.list_items()

    # Build response BEFORE commit/close if it needs db access
    result = [ItemResponse.from_orm(item) for item in items]

    db.commit()
    db.close()
    return result
```

#### Pattern C: Conditional Returns
```python
async def create_item(request: ItemCreate, db: Session = Depends(get_db)):
    try:
        service = ItemService(db)
        item = await service.create_item(request)

        result = ItemResponse.from_orm(item)
        db.commit()
        db.close()
        return result
    except ValidationError as e:
        # Don't commit/close here - let exception propagate
        raise HTTPException(status_code=400, detail=str(e))
```

#### Pattern D: Void Returns (DELETE endpoints)
```python
async def delete_item(item_id: str, db: Session = Depends(get_db)):
    service = ItemService(db)
    await service.delete_item(item_id)

    db.commit()
    db.close()
    return {"status": "success", "message": f"Item {item_id} deleted"}
```

### Incorrect Patterns to Fix

#### Anti-Pattern 1: Direct Return
```python
# WRONG: Transaction held during serialization
async def get_item(item_id: str, db: Session = Depends(get_db)):
    service = ItemService(db)
    return await service.get_item(item_id)  # No commit/close!
```

#### Anti-Pattern 2: Commit Without Close
```python
# WRONG: Connection not returned to pool
async def create_item(request: ItemCreate, db: Session = Depends(get_db)):
    service = ItemService(db)
    item = await service.create_item(request)
    db.commit()  # Missing db.close()!
    return item
```

#### Anti-Pattern 3: Close Without Commit
```python
# WRONG: Transaction implicitly rolled back
async def update_item(item_id: str, db: Session = Depends(get_db)):
    service = ItemService(db)
    item = await service.update_item(item_id)
    db.close()  # Missing db.commit()!
    return item
```

## Files to Audit

### Priority 1: Routers (High Traffic)
- `mcpgateway/routers/tokens.py`
- `mcpgateway/routers/teams.py`
- `mcpgateway/routers/rbac.py`
- `mcpgateway/routers/email_auth.py`
- `mcpgateway/routers/sso.py`
- `mcpgateway/routers/oauth_router.py`
- `mcpgateway/routers/llm_config_router.py`

### Priority 2: Main API (Core Operations)
- `mcpgateway/main.py` - REST endpoints and RPC handlers

### Priority 3: Admin API
- `mcpgateway/admin.py` - Admin UI and management endpoints

## Verification

After identifying and fixing issues, verify with:

### 1. Syntax Check
```bash
python3 -c "import ast; ast.parse(open('mcpgateway/routers/FILE.py').read())"
```

### 2. Unit Tests
```bash
pytest tests/unit/mcpgateway/routers/ -v
```

### 3. PostgreSQL Connection Monitoring (During Load Test)
```bash
# Check for idle-in-transaction connections
docker exec postgres psql -U postgres -d mcp -c "
SELECT state, COUNT(*)
FROM pg_stat_activity
WHERE datname = 'mcp'
GROUP BY state;"

# Target: 0 connections in 'idle in transaction' state
```

## Output Format

Report findings as:

```markdown
## Audit Results

### Missing db.commit(); db.close()

| File | Function | Line | Issue |
|------|----------|------|-------|
| routers/example.py | get_item | 123 | Missing both commit and close |
| routers/example.py | update_item | 456 | Missing close (has commit) |

### Already Correct
- routers/teams.py - All 18 endpoints verified ✓
- routers/rbac.py - All 12 endpoints verified ✓

### Exceptions (No Fix Needed)
- Functions that don't use db parameter (marked as `_db`)
- Functions that delegate to other services with own session management
```

## Notes

- The `get_db` dependency's `finally` block is a safety net, not a substitute for explicit cleanup
- Always commit even for read operations - it ensures clean transaction state
- The order must be: `db.commit()` THEN `db.close()`
- For endpoints with `_db: Session = Depends(get_db)` (underscore prefix), the parameter is unused - no action needed