Skip to content

Commit cec38ee

Browse files
committed
feat: production readiness hardening
Security: add auth to all container routes, production env validation (fails startup if secrets missing), tighten CORS methods/headers, catch-all exception handler, fix OAuth redirect to use APP_BASE_URL. Infrastructure: complete production docker-compose with docker socket, skills mount, health checks, resource limits, required POSTGRES_PASSWORD. Non-root API container, multi-worker uvicorn, nginx security headers. Resilience: graceful shutdown stops running containers, boot-time reconciliation marks stale sessions as FAILED, configurable container TTL via CONTAINER_TTL_SECONDS setting. Observability: health check verifies DB connectivity (503 if degraded), complete .env.example with all production variables documented.
1 parent 75280ac commit cec38ee

14 files changed

Lines changed: 234 additions & 24 deletions

File tree

.env.example

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ DATABASE_URL=postgresql+asyncpg://helprs:helprs@localhost:5432/helprs
66
SECRET_KEY=
77
# Generate with: python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())"
88
FERNET_KEY=
9+
# Required in production (ENVIRONMENT=production)
10+
ADMIN_PASSWORD=
911

1012
# GitHub App
1113
GITHUB_APP_ID=
@@ -22,3 +24,20 @@ VITE_API_URL=http://localhost:8000
2224

2325
# Public URL used by the backend to build user-facing links (e.g. PR-comment session links)
2426
APP_BASE_URL=http://localhost:5173
27+
28+
# CORS — JSON array of allowed origins
29+
CORS_ORIGINS=["http://localhost:5173"]
30+
31+
# Environment: "development" or "production"
32+
# Production enforces non-empty secrets and disables admin auto-auth
33+
ENVIRONMENT=development
34+
35+
# Container orchestration
36+
CONTAINER_TTL_SECONDS=900
37+
UVICORN_WORKERS=4
38+
39+
# Docker host path to skills/ directory (required for Docker-in-Docker volume mounts)
40+
# SKILLS_HOST_PATH=/absolute/path/to/skills
41+
42+
# Postgres credentials (used by docker-compose.prod.yml)
43+
# POSTGRES_PASSWORD=

CLAUDE.md

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ infra/
5656
- **Admin panel**: SQLAdmin at `/admin`, configured in `admin/views.py`
5757
- **Dashboard**: user-facing installation management at `/installations` -- installation list, session history, session replay. Authenticated users redirect from `/` to `/installations`. SQLAdmin remains at `/admin` as superadmin escape hatch.
5858
- **Cross-module queries**: installation module queries `ContainerSession` model directly (inline import in service functions) for session counts and lists. This avoids circular imports while keeping the API surface on the installation router.
59+
- **Auth on all REST routes**: identity and installation routers use `Depends(get_current_user)`, container router uses it too. The webhook handler bypasses REST routes entirely — it calls `create_session()` directly (DB record only, no container start). Container start happens when the authenticated frontend calls the REST endpoint.
60+
- **Production env validation**: `Settings` has a `model_validator` that enforces non-empty secrets when `ENVIRONMENT=production`. Tests use `ENVIRONMENT=test` to skip this.
61+
- **Graceful lifecycle**: lifespan reconciles stale RUNNING/PENDING sessions on boot (marks FAILED), and stops all running containers on shutdown. Periodic cleanup uses configurable `CONTAINER_TTL_SECONDS` from settings.
5962

6063
## Skills
6164

@@ -88,11 +91,8 @@ cd apps/api && uv run alembic revision --autogenerate -m "description" # New mi
8891

8992
## Environment
9093

91-
Required `.env` at repo root (see docker-compose.yml):
92-
- `DATABASE_URL` — Postgres connection string
93-
- `SECRET_KEY` — app secret
94-
- `GITHUB_APP_ID`, `GITHUB_WEBHOOK_SECRET` — GitHub App config
95-
- `FERNET_KEY` — encryption key for stored credentials
94+
Required `.env` at repo root — see `.env.example` for all variables with generation instructions.
95+
Key additions for production: `ENVIRONMENT=production`, `ADMIN_PASSWORD`, `CORS_ORIGINS`, `CONTAINER_TTL_SECONDS`, `UVICORN_WORKERS`.
9696

9797
## Gotchas
9898

@@ -113,6 +113,8 @@ Required `.env` at repo root (see docker-compose.yml):
113113
- **Shallow clone + `gh pr checkout --detach`**: `gh pr checkout` (without `--detach`) fails on `--depth=1` clones because git can't set up tracking branches from shallow refs. Always use `--detach` — containers don't need tracking branches, just files on disk.
114114
- **Installation IDs in URLs**: frontend routes (`/installations/:id`) use `github_installation_id` (integer, e.g. `123093268`), NOT the internal UUID. The API installation endpoints also expect the GitHub integer ID.
115115
- **Fast-failing container race**: if a container exits before the SSE stream fully drains, the client may disconnect before `mark_completed()` runs, leaving the session stuck as RUNNING with 0 persisted events. The 5-minute cleanup task marks these as TIMEOUT. Root cause: generator cancellation on client disconnect skips the post-stream `mark_completed` call in `_event_stream()`.
116+
- **Flaky dispatcher tests**: `test_issues_opened_is_ignored_and_logged` and `test_pull_request_closed_is_ignored` fail when run as part of the full suite due to structlog `configure_logging()` state contamination from `create_app()` in earlier tests. They pass in isolation.
117+
- **Multi-worker background tasks**: with `--workers N`, each uvicorn worker runs its own lifespan (webhook reaper + container cleanup). Both are idempotent: reaper uses atomic row-level claim (`mark_processing`), cleanup suppresses double-stop exceptions.
116118

117119
## Key Decisions
118120

@@ -122,3 +124,5 @@ Required `.env` at repo root (see docker-compose.yml):
122124
- **Dashboard over SQLAdmin**: user-facing operations (installation list, session history, token config) go through the dashboard UI; SQLAdmin is the superadmin escape hatch
123125
- **Open source target**: designed for self-hosting with own Claude licenses
124126
- **Post-results to PR**: after session completion, the API can post score card as a PR comment — opt-in per installation via `post_results_to_pr` boolean; extraction and formatting in `container/pr_comment.py`, triggered in `_event_stream()` after `mark_completed()`
127+
- **Coolify deployment**: TLS terminates at the Coolify reverse proxy, not in nginx. Nginx serves the SPA with security headers but no HTTPS config. SSE `X-Accel-Buffering: no` header is set by the API, not nginx.
128+
- **Non-root API container**: production Dockerfile uses `appuser`. Port 8000 > 1024 so no privilege needed. Docker socket mount still grants Docker access regardless of USER.

apps/api/src/helprs/core/config.py

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
from functools import lru_cache
44

5-
from pydantic import field_validator
5+
from pydantic import field_validator, model_validator
66
from pydantic_settings import BaseSettings, SettingsConfigDict
77

88

@@ -42,6 +42,9 @@ class Settings(BaseSettings):
4242
# string is for link composition. See deferred-work.md nice-to-have #11.
4343
APP_BASE_URL: str = "http://localhost:5173"
4444

45+
# Container orchestration
46+
CONTAINER_TTL_SECONDS: int = 900 # 15 minutes
47+
4548
# Environment
4649
ENVIRONMENT: str = "development"
4750

@@ -61,6 +64,28 @@ def validate_fernet_key(cls, v: str) -> str:
6164
) from e
6265
return v
6366

67+
@model_validator(mode="after")
68+
def validate_production_secrets(self) -> "Settings":
69+
"""Enforce that critical secrets are set when ENVIRONMENT is production."""
70+
if self.ENVIRONMENT != "production":
71+
return self
72+
missing: list[str] = []
73+
if not self.ADMIN_PASSWORD:
74+
missing.append("ADMIN_PASSWORD")
75+
if len(self.SECRET_KEY) < 32:
76+
missing.append("SECRET_KEY (must be >= 32 characters)")
77+
if not self.GITHUB_WEBHOOK_SECRET:
78+
missing.append("GITHUB_WEBHOOK_SECRET")
79+
if not self.GITHUB_APP_PRIVATE_KEY:
80+
missing.append("GITHUB_APP_PRIVATE_KEY")
81+
if not self.GITHUB_CLIENT_ID:
82+
missing.append("GITHUB_CLIENT_ID")
83+
if not self.GITHUB_CLIENT_SECRET:
84+
missing.append("GITHUB_CLIENT_SECRET")
85+
if missing:
86+
raise ValueError(f"Production environment requires: {', '.join(missing)}")
87+
return self
88+
6489

6590
@lru_cache
6691
def get_settings() -> Settings:

apps/api/src/helprs/core/middleware.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,8 @@ def setup_middleware(app: FastAPI, settings: Settings) -> None:
9595
CORSMiddleware,
9696
allow_origins=settings.CORS_ORIGINS,
9797
allow_credentials=True,
98-
allow_methods=["*"],
99-
allow_headers=["*"],
98+
allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"],
99+
allow_headers=["Authorization", "Content-Type", "X-Request-ID", "Last-Event-ID"],
100100
)
101101

102102
# Request logging

apps/api/src/helprs/main.py

Lines changed: 50 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
from contextlib import asynccontextmanager
66

77
import structlog
8-
from fastapi import APIRouter, FastAPI
8+
from fastapi import APIRouter, FastAPI, Request
9+
from fastapi.responses import JSONResponse
910

1011
from helprs.core.config import get_settings
1112
from helprs.core.database import (
@@ -113,13 +114,14 @@ async def _run_container_cleanup(
113114
"""
114115
from helprs.modules.container.service import AioDockerClient, cleanup_expired
115116

117+
ttl = get_settings().CONTAINER_TTL_SECONDS
116118
try:
117119
while True:
118120
await asyncio.sleep(interval_seconds)
119121
try:
120122
docker = AioDockerClient()
121123
async with app.state.session_factory() as db:
122-
cleaned = await cleanup_expired(db, docker)
124+
cleaned = await cleanup_expired(db, docker, ttl_seconds=ttl)
123125
await db.commit()
124126
if cleaned:
125127
logger.info("container_cleanup_cycle", cleaned=cleaned)
@@ -164,6 +166,18 @@ async def lifespan(app: FastAPI):
164166
# server starts serving traffic immediately (AC #2).
165167
await _replay_pending_webhook_events(app)
166168

169+
# Reconcile container sessions left in RUNNING/PENDING by a
170+
# prior crash — mark them FAILED immediately rather than
171+
# waiting for TTL expiry.
172+
try:
173+
from helprs.modules.container.service import reconcile_stale_sessions
174+
175+
async with session_factory() as db:
176+
await reconcile_stale_sessions(db)
177+
await db.commit()
178+
except Exception:
179+
logger.exception("session_reconciliation_failed")
180+
167181
# Periodic reaper: handles rows that get stuck mid-run (e.g.
168182
# mark_processed commit failure) without waiting for the next
169183
# restart.
@@ -188,6 +202,20 @@ async def lifespan(app: FastAPI):
188202
if tracked:
189203
await asyncio.gather(*tracked, return_exceptions=True)
190204

205+
# Stop all running containers before shutting down.
206+
try:
207+
from helprs.modules.container.service import AioDockerClient, cleanup_all_running
208+
209+
docker = AioDockerClient()
210+
async with session_factory() as db:
211+
stopped = await cleanup_all_running(db, docker)
212+
await db.commit()
213+
if stopped:
214+
logger.info("shutdown_containers_stopped", count=stopped)
215+
await docker.close()
216+
except Exception:
217+
logger.exception("shutdown_container_cleanup_failed")
218+
191219
clear_session_factory()
192220
await engine.dispose()
193221

@@ -201,6 +229,15 @@ async def lifespan(app: FastAPI):
201229
# Exception handlers
202230
app.add_exception_handler(DomainError, domain_exception_handler)
203231

232+
async def _unhandled_exception_handler(request: Request, exc: Exception) -> JSONResponse:
233+
logger.exception("unhandled_exception", path=request.url.path)
234+
return JSONResponse(
235+
status_code=500,
236+
content={"error": "internal_error", "message": "An unexpected error occurred"},
237+
)
238+
239+
app.add_exception_handler(Exception, _unhandled_exception_handler)
240+
204241
# Middleware (CORS, logging, rate limiting)
205242
setup_middleware(app, settings)
206243

@@ -222,7 +259,17 @@ async def lifespan(app: FastAPI):
222259
# Health check
223260
@app.get("/health")
224261
async def health_check():
225-
return {"status": "ok"}
262+
from sqlalchemy import text
263+
264+
try:
265+
async with app.state.session_factory() as db:
266+
await db.execute(text("SELECT 1"))
267+
return {"status": "ok", "db": "ok"}
268+
except Exception:
269+
return JSONResponse(
270+
status_code=503,
271+
content={"status": "degraded", "db": "unreachable"},
272+
)
226273

227274
return app
228275

apps/api/src/helprs/modules/container/router.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33
import json
44
from uuid import UUID
55

6-
from fastapi import APIRouter, Request
6+
from fastapi import APIRouter, Depends, Request
77
from fastapi.responses import StreamingResponse
88

99
from helprs.core.database import get_db_context
10-
from helprs.core.dependencies import DbSession, GetSettings
10+
from helprs.core.dependencies import DbSession, GetSettings, get_current_user
1111
from helprs.core.exceptions import NotFoundError
1212
from helprs.core.middleware import limiter
1313
from helprs.core.security import fernet_decrypt
@@ -49,6 +49,7 @@ async def create_container_session(
4949
request: Request,
5050
db: DbSession,
5151
settings: GetSettings,
52+
user=Depends(get_current_user), # noqa: B008
5253
):
5354
"""Create a container session and start the container.
5455
@@ -87,6 +88,7 @@ async def create_container_session(
8788
pr_number=body.pr_number,
8889
repo_full_name=body.repo_full_name,
8990
skill_name=body.skill_name,
91+
user_id=user.id,
9092
)
9193

9294
# Start the container
@@ -112,6 +114,7 @@ async def get_container_session(
112114
session_id: UUID,
113115
request: Request,
114116
db: DbSession,
117+
user=Depends(get_current_user), # noqa: B008
115118
):
116119
"""Get the current status of a container session."""
117120
cs = await get_session_or_404(db, session_id)
@@ -125,6 +128,7 @@ async def stream_container_output(
125128
session_id: UUID,
126129
request: Request,
127130
db: DbSession,
131+
user=Depends(get_current_user), # noqa: B008
128132
offset: int = 0,
129133
):
130134
"""SSE endpoint streaming container stdout/stderr.
@@ -189,6 +193,7 @@ async def get_session_events_endpoint(
189193
session_id: UUID,
190194
request: Request,
191195
db: DbSession,
196+
user=Depends(get_current_user), # noqa: B008
192197
):
193198
"""Retrieve persisted stream-json events for a session.
194199
@@ -210,6 +215,7 @@ async def send_session_message(
210215
body: SendMessageRequest,
211216
request: Request,
212217
db: DbSession,
218+
user=Depends(get_current_user), # noqa: B008
213219
):
214220
"""Send a user message to a running container session.
215221
@@ -235,6 +241,7 @@ async def stop_container_session(
235241
session_id: UUID,
236242
request: Request,
237243
db: DbSession,
244+
user=Depends(get_current_user), # noqa: B008
238245
):
239246
"""Stop a running container session."""
240247
docker = _get_docker_client()

apps/api/src/helprs/modules/container/service.py

Lines changed: 59 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@
3434

3535
logger = structlog.get_logger()
3636

37-
# Container TTL: 15 minutes maximum
38-
CONTAINER_TTL_SECONDS = 15 * 60
37+
# Default container TTL (overridden by Settings.CONTAINER_TTL_SECONDS)
38+
_DEFAULT_CONTAINER_TTL_SECONDS = 15 * 60
3939

4040
# Base image for the claude-runner container
4141
CLAUDE_RUNNER_IMAGE = "helprs/claude-runner:latest"
@@ -546,6 +546,7 @@ async def mark_completed(
546546
db: AsyncSession,
547547
session_id: UUID,
548548
docker: DockerClient,
549+
ttl_seconds: int = _DEFAULT_CONTAINER_TTL_SECONDS,
549550
) -> ContainerSession:
550551
"""Wait for the container to finish, capture exit code, clean up."""
551552
cs = await get_session_or_404(db, session_id)
@@ -556,7 +557,7 @@ async def mark_completed(
556557
try:
557558
exit_code = await asyncio.wait_for(
558559
docker.wait_container(cs.container_id),
559-
timeout=CONTAINER_TTL_SECONDS,
560+
timeout=ttl_seconds,
560561
)
561562
cs.status = ContainerStatus.COMPLETED if exit_code == 0 else ContainerStatus.FAILED
562563
except TimeoutError:
@@ -580,12 +581,13 @@ async def mark_completed(
580581
async def cleanup_expired(
581582
db: AsyncSession,
582583
docker: DockerClient,
584+
ttl_seconds: int = _DEFAULT_CONTAINER_TTL_SECONDS,
583585
) -> int:
584586
"""Find sessions past TTL and destroy their containers.
585587

586588
Returns the number of sessions cleaned up.
587589
"""
588-
cutoff = datetime.now(UTC).timestamp() - CONTAINER_TTL_SECONDS
590+
cutoff = datetime.now(UTC).timestamp() - ttl_seconds
589591
cutoff_dt = datetime.fromtimestamp(cutoff, tz=UTC)
590592

591593
result = await db.execute(
@@ -617,3 +619,56 @@ async def cleanup_expired(
617619
await logger.ainfo("expired_sessions_cleaned", count=cleaned)
618620

619621
return cleaned
622+
623+
624+
async def cleanup_all_running(
625+
db: AsyncSession,
626+
docker: DockerClient,
627+
) -> int:
628+
"""Stop ALL running/pending sessions. Used during graceful shutdown."""
629+
result = await db.execute(
630+
select(ContainerSession).where(
631+
ContainerSession.status.in_([ContainerStatus.RUNNING, ContainerStatus.PENDING]),
632+
)
633+
)
634+
sessions = list(result.scalars().all())
635+
636+
cleaned = 0
637+
for cs in sessions:
638+
if cs.container_id:
639+
with contextlib.suppress(Exception):
640+
await docker.stop_container(cs.container_id)
641+
await docker.remove_container(cs.container_id, force=True)
642+
cs.status = ContainerStatus.COMPLETED
643+
cs.completed_at = datetime.now(UTC)
644+
cleaned += 1
645+
646+
if cleaned:
647+
await db.flush()
648+
await logger.ainfo("shutdown_sessions_stopped", count=cleaned)
649+
650+
return cleaned
651+
652+
653+
async def reconcile_stale_sessions(db: AsyncSession) -> int:
654+
"""Mark any RUNNING/PENDING sessions as FAILED on boot.
655+
656+
After a crash, containers are gone but DB records still show RUNNING.
657+
This reconciles them immediately rather than waiting for TTL expiry.
658+
"""
659+
result = await db.execute(
660+
select(ContainerSession).where(
661+
ContainerSession.status.in_([ContainerStatus.RUNNING, ContainerStatus.PENDING]),
662+
)
663+
)
664+
stale = list(result.scalars().all())
665+
666+
for cs in stale:
667+
cs.status = ContainerStatus.FAILED
668+
cs.completed_at = datetime.now(UTC)
669+
670+
if stale:
671+
await db.flush()
672+
await logger.ainfo("stale_sessions_reconciled", count=len(stale))
673+
674+
return len(stale)

0 commit comments

Comments
 (0)