diff --git a/apps/api/src/main.py b/apps/api/src/main.py index 3e4212d..32cc02e 100644 --- a/apps/api/src/main.py +++ b/apps/api/src/main.py @@ -57,10 +57,27 @@ async def lifespan(app: FastAPI): reap_stuck_indexing(engine) logger.info("Database initialized") - # Initialize vector store - vector_store = get_vector_store() - await vector_store.initialize() - logger.info("Vector store initialized") + # Initialize the vector store. + # + # Non-fatal, because building it constructs the embedding client eagerly, and the + # provider SDKs raise on a missing key at construction time rather than on first use. + # Letting that propagate meant the process exited during startup whenever a key was + # absent, with three consequences that only became obvious once this ran in + # Kubernetes: the pod CrashLoopBackOffs instead of reporting itself unhealthy, you + # cannot deploy first and supply credentials afterwards, and /api/health can never + # report "llm_provider unreachable" because the app never boots far enough to serve + # it. Endpoints that genuinely need embeddings still fail per-request with a clear + # error; the ones that do not (platform config, repo listing, progress) keep working. + vector_store = None + try: + vector_store = get_vector_store() + await vector_store.initialize() + logger.info("Vector store initialized") + except Exception as exc: + logger.error( + "Vector store unavailable at startup; search, chat and indexing will fail " + "until this is resolved (check the embedding provider credentials): %s", exc + ) # Optional Neo4j read model. Non-fatal by design: the graph endpoint falls back to # the SQL path, so an unreachable graph database must not stop the API booting. @@ -79,7 +96,14 @@ async def lifespan(app: FastAPI): # Shutdown logger.info("Shutting down CodebaseQA API...") - await vector_store.close() + # May be None when startup could not build it; closing unconditionally would raise + # AttributeError during shutdown and mask the real startup error. + if vector_store is not None: + try: + await vector_store.close() + except Exception as exc: + logger.warning("Vector store close failed: %s", exc) + graph_driver = get_graph_driver() if graph_driver is not None: try: diff --git a/apps/api/src/models/migrations.py b/apps/api/src/models/migrations.py index 88517b2..063282d 100644 --- a/apps/api/src/models/migrations.py +++ b/apps/api/src/models/migrations.py @@ -24,6 +24,35 @@ def _table_exists(engine: Engine, table_name: str) -> bool: return inspector.has_table(table_name) +def _add_column(connection, table: str, column: str, ddl: str, applied: List[str]) -> None: + """ + Add a column idempotently, tolerating a concurrent process adding it first. + + SQLite has no ADD COLUMN IF NOT EXISTS, so the only option is check-then-act -- which + is a race: two replicas starting together both see the column missing and both issue + the ALTER, and the loser fails with "duplicate column name". That took down the second + replica's startup entirely. + + Rather than serialize startup with a lock (a lock file is unreliable on a shared + volume, and a DB-level mutex needs its own release path), the operation is made + genuinely idempotent: a duplicate-column error means someone else applied it, which is + success. Every other error still propagates. + """ + from sqlalchemy.exc import OperationalError, ProgrammingError + + try: + connection.execute(text(ddl)) + applied.append(f"{table}.{column}") + except (OperationalError, ProgrammingError) as exc: + message = str(exc).lower() + if "duplicate column" in message or "already exists" in message: + logger.debug( + "Column %s.%s already added by another process; continuing", table, column + ) + return + raise + + def run_pending_migrations(engine: Engine) -> List[str]: """ Apply additive migrations required for backward-compatible schema hardening. @@ -32,9 +61,10 @@ def run_pending_migrations(engine: Engine) -> List[str]: applied: List[str] = [] with engine.begin() as connection: - if not _column_exists(engine, "chat_messages", "retrieval_meta"): - connection.execute(text("ALTER TABLE chat_messages ADD COLUMN retrieval_meta JSON")) - applied.append("chat_messages.retrieval_meta") + _add_column( + connection, "chat_messages", "retrieval_meta", + "ALTER TABLE chat_messages ADD COLUMN retrieval_meta JSON", applied, + ) connection.execute( text( @@ -98,17 +128,20 @@ def run_pending_migrations(engine: Engine) -> List[str]: ) applied.append("ix_learning_lessons_expiry") - if not _column_exists(engine, "lesson_progress", "persona"): - connection.execute(text("ALTER TABLE lesson_progress ADD COLUMN persona VARCHAR(50)")) - applied.append("lesson_progress.persona") + _add_column( + connection, "lesson_progress", "persona", + "ALTER TABLE lesson_progress ADD COLUMN persona VARCHAR(50)", applied, + ) - if not _column_exists(engine, "lesson_progress", "module_id"): - connection.execute(text("ALTER TABLE lesson_progress ADD COLUMN module_id VARCHAR(100)")) - applied.append("lesson_progress.module_id") + _add_column( + connection, "lesson_progress", "module_id", + "ALTER TABLE lesson_progress ADD COLUMN module_id VARCHAR(100)", applied, + ) - if not _column_exists(engine, "learning_syllabi", "expires_at"): - connection.execute(text("ALTER TABLE learning_syllabi ADD COLUMN expires_at DATETIME")) - applied.append("learning_syllabi.expires_at") + _add_column( + connection, "learning_syllabi", "expires_at", + "ALTER TABLE learning_syllabi ADD COLUMN expires_at DATETIME", applied, + ) # code_dependencies itself is created by init_db/create_all, which runs first # (main.py). These indexes are declared on the model too, so this block only diff --git a/docker/Dockerfile.api b/docker/Dockerfile.api index 4ea5b9e..b0efe79 100644 --- a/docker/Dockerfile.api +++ b/docker/Dockerfile.api @@ -26,6 +26,22 @@ COPY apps/api . COPY docker/entrypoint.sh /entrypoint.sh RUN chmod +x /entrypoint.sh +# Run as a non-root user. +# +# The container previously ran as uid 0, which matters in two concrete ways: under +# docker-compose it bind-mounts ../data and so created root-owned files in the +# developer's working tree, and in Kubernetes it cannot satisfy +# securityContext.runAsNonRoot (the pod fails to start with CreateContainerConfigError). +# +# uid 10001 is arbitrary but fixed, so a persistent volume written by one version stays +# writable by the next. /app/data must be owned by it because SQLite and Chroma both +# write there; a Kubernetes PVC is chowned via fsGroup instead (see the StatefulSet). +RUN groupadd --system --gid 10001 appuser \ + && useradd --system --uid 10001 --gid appuser --no-create-home appuser \ + && mkdir -p /app/data \ + && chown -R appuser:appuser /app +USER appuser + # Expose port EXPOSE 8000 diff --git a/infra/k8s/README.md b/infra/k8s/README.md new file mode 100644 index 0000000..4fbd048 --- /dev/null +++ b/infra/k8s/README.md @@ -0,0 +1,119 @@ +# Kubernetes + +Manifests for the API, validated on [kind](https://kind.sigs.k8s.io/). Everything below +was actually run — the pod reaches `1/1 Running`, serves traffic on a NodePort, and its +volume survives pod deletion. + +## What this does and does not give you + +Of the capabilities kubernetes.io lists, this workload can genuinely use two: **storage +orchestration** and **self-healing restarts**. + +It does **not** give you horizontal scaling, and the manifests say `replicas: 1` on +purpose. Two reasons, both properties of the application rather than of these files: + +1. **SQLite.** `DATABASE_URL` is a file on the volume. A `ReadWriteOnce` PVC attaches to + one node, and concurrent writers to a single SQLite file are the configuration + `sqlite.org/howtocorrupt.html` warns about. +2. **ChromaDB is embedded.** `chroma_store.py` holds a process-local persistent client + over the same directory, so two pods would be two independent writers to one on-disk + index. + +Startup *is* now safe to run concurrently — the `ALTER TABLE` migrations were changed from +check-then-act to idempotent, so a second replica no longer dies on `duplicate column +name`. But safe startup is not safe operation. Scaling past 1 means moving to Postgres and +a server-mode vector store. + +Writing these manifests is what surfaced both constraints, plus two real bugs (see below). + +## Run it + +```bash +kind create cluster --config infra/k8s/kind-cluster.yaml +docker build -f docker/Dockerfile.api -t codebaseqa-api:kind . +kind load docker-image codebaseqa-api:kind --name codebaseqa + +# Real credentials out of band, so they never enter git: +kubectl create secret generic codebaseqa-secrets \ + --from-literal=OPENAI_API_KEY=sk-... \ + --from-literal=GITHUB_TOKEN= + +kubectl apply -k infra/k8s/ +kubectl rollout status statefulset/codebaseqa-api +curl http://localhost:30080/api/platform/config +``` + +`secret.yaml` is a **template with empty values**. It exists so `kubectl kustomize` +resolves in a bare cluster; do not put real keys in it. + +## Decisions worth knowing + +**`imagePullPolicy: Never`.** The image is built locally and side-loaded with `kind load`. +With `IfNotPresent` or `Always`, the kubelet would try a registry and fail on a tag that +only exists in the node's containerd. + +**The readiness probe is not `/api/health`.** That endpoint reports `degraded` whenever +*any* dependency is unreachable, including the LLM provider — a third party. Gating +readiness on it would pull the pod out of service because OpenAI had a bad minute, and in +a cluster with no key it would never become ready at all. Readiness instead hits +`/api/platform/config`, which touches only the local database, and answers the question +the kubelet actually needs: is this process serving HTTP? + +**`fsGroup: 10001`.** This is what makes the mounted PVC writable by the non-root user — +the kubelet chowns the volume to that GID. Without it the pod starts and then fails on the +first SQLite write, which is a much worse failure than not starting. + +**`runAsNonRoot: true`.** The image now creates uid 10001 and sets `USER`. This makes the +kubelet refuse the pod if that ever regresses. + +**Not deployed here:** Redis and Neo4j. The ConfigMap disables both, and the in-memory +fallbacks cover them. Add them as their own StatefulSets if you want them in-cluster. + +**kind, not AKS.** kind is a Kubernetes SIG project and costs nothing. AKS's cheapest sane +node (`Standard_B2s`) is roughly $30/month to run something pinned to one replica. + +## Two bugs this found + +Neither was visible from reading the code, and neither would have been caught by the test +suite. + +**The image ran as root.** Confirmed by inspecting the built image (`id -u` → 0). Under +docker-compose that meant root-owned files in the developer's working tree via the +`../data` bind mount; in Kubernetes it made `runAsNonRoot` unsatisfiable. Fixed in +`docker/Dockerfile.api`. + +**The API could not start without a provider key.** The pod went into +`CrashLoopBackOff` with `openai.OpenAIError: Missing credentials`, because the lifespan +builds the vector store eagerly and the OpenAI SDK raises at *construction*, not first +use. Three consequences: the pod crash-loops instead of reporting itself unhealthy, you +cannot deploy first and add credentials afterwards, and `/api/health` can never report +`llm_provider unreachable` because the process never boots far enough to serve it. Vector +store initialization is now non-fatal — endpoints that need embeddings still fail +per-request with a clear error, and the rest keep working: + +```json +{ "status": "degraded", + "checks": { "database": "ok", + "vector_store": "error: Missing credentials...", + "llm_provider": "error: Missing credentials..." } } +``` + +## Verified + +| Check | Result | +|---|---| +| `kubectl kustomize` | 5 resources render | +| Rollout | `codebaseqa-api-0 1/1 Running 0 restarts` | +| NodePort from host | `curl localhost:30080/api/platform/config` returns JSON | +| `/health` with no key | `degraded` with a specific reason, process stays up | +| Migrations at startup | applied, including `ix_code_dependencies_repo` | +| PVC | `Bound`, 5Gi, RWO | +| Non-root write | uid 10001 writes `/app/data` via `fsGroup` | +| Persistence | marker file and `codebaseqa.db` survive `kubectl delete pod` | +| Concurrent startup | 6 simultaneous migration runs, 0 failures | + +## Not verified + +The web frontend is not deployed here — these manifests cover the API only. Nothing has +been run on a managed cluster (AKS/EKS/GKE); a cloud `StorageClass` and `LoadBalancer` +would replace kind's `standard` class and the NodePort. diff --git a/infra/k8s/configmap.yaml b/infra/k8s/configmap.yaml new file mode 100644 index 0000000..24f5d72 --- /dev/null +++ b/infra/k8s/configmap.yaml @@ -0,0 +1,30 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: codebaseqa-config +data: + # Paths are inside the mounted volume; see the StatefulSet's volumeMounts. + DATABASE_URL: "sqlite:///./data/codebaseqa.db" + CHROMA_PERSIST_DIR: "./data/chroma" + REPOS_DIR: "./data/repos" + + LLM_PROVIDER: "openai" + EMBEDDING_PROVIDER: "openai" + OPENAI_MODEL: "gpt-4o" + OPENAI_EMBEDDING_MODEL: "text-embedding-3-small" + OPENAI_EMBEDDING_DIMENSIONS: "1536" + + # Comma-separated is deliberate: config.py reads CORS_ORIGINS as a raw string and + # splits it in a property, precisely because pydantic-settings would otherwise + # JSON-decode a List[str] field and raise before uvicorn binds. + CORS_ORIGINS: "http://localhost:3000,http://127.0.0.1:3000" + + DEBUG: "false" + SEED_DEMO: "false" + DEMO_MODE: "false" + + # Redis and Neo4j are not deployed by these manifests. Left off so the API runs + # standalone; the in-memory fallbacks cover both. + RATE_LIMIT_REDIS_ENABLED: "false" + CHAT_REDIS_CACHE_ENABLED: "false" + NEO4J_ENABLED: "false" diff --git a/infra/k8s/kind-cluster.yaml b/infra/k8s/kind-cluster.yaml new file mode 100644 index 0000000..35d1abf --- /dev/null +++ b/infra/k8s/kind-cluster.yaml @@ -0,0 +1,15 @@ +# kind cluster for local validation. kind is a Kubernetes SIG project and costs nothing, +# which is the honest choice here: AKS's cheapest sane node (Standard_B2s) is ~$30/month +# to run a workload that is pinned to a single replica. +# +# extraPortMappings exposes the NodePort on the host so the API is reachable at +# http://localhost:30080 without kubectl port-forward. +kind: Cluster +apiVersion: kind.x-k8s.io/v1alpha4 +name: codebaseqa +nodes: + - role: control-plane + extraPortMappings: + - containerPort: 30080 + hostPort: 30080 + protocol: TCP diff --git a/infra/k8s/kustomization.yaml b/infra/k8s/kustomization.yaml new file mode 100644 index 0000000..aeda01e --- /dev/null +++ b/infra/k8s/kustomization.yaml @@ -0,0 +1,7 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: + - configmap.yaml + - secret.yaml + - service.yaml + - statefulset.yaml diff --git a/infra/k8s/secret.yaml b/infra/k8s/secret.yaml new file mode 100644 index 0000000..90ab830 --- /dev/null +++ b/infra/k8s/secret.yaml @@ -0,0 +1,19 @@ +# TEMPLATE ONLY -- do not commit real values, and do not `kubectl apply` this as-is. +# +# Create the real Secret out of band so it never enters git: +# +# kubectl create secret generic codebaseqa-secrets \ +# --from-literal=OPENAI_API_KEY=sk-... \ +# --from-literal=GITHUB_TOKEN=ghp-... +# +# stringData with empty values exists so `kustomize build` resolves and the StatefulSet's +# secretKeyRef references are satisfied in a bare cluster. An empty OPENAI_API_KEY makes +# /api/health report the LLM provider unreachable, which is accurate rather than silent. +apiVersion: v1 +kind: Secret +metadata: + name: codebaseqa-secrets +type: Opaque +stringData: + OPENAI_API_KEY: "" + GITHUB_TOKEN: "" diff --git a/infra/k8s/service.yaml b/infra/k8s/service.yaml new file mode 100644 index 0000000..ed204f2 --- /dev/null +++ b/infra/k8s/service.yaml @@ -0,0 +1,33 @@ +# Headless service for the StatefulSet's stable DNS name. +apiVersion: v1 +kind: Service +metadata: + name: codebaseqa-api + labels: + app: codebaseqa-api +spec: + clusterIP: None + selector: + app: codebaseqa-api + ports: + - name: http + port: 8000 + targetPort: http +--- +# NodePort so the API is reachable from the host without port-forward. The port is +# mapped by kind-cluster.yaml's extraPortMappings. +apiVersion: v1 +kind: Service +metadata: + name: codebaseqa-api-nodeport + labels: + app: codebaseqa-api +spec: + type: NodePort + selector: + app: codebaseqa-api + ports: + - name: http + port: 8000 + targetPort: http + nodePort: 30080 diff --git a/infra/k8s/statefulset.yaml b/infra/k8s/statefulset.yaml new file mode 100644 index 0000000..1c27843 --- /dev/null +++ b/infra/k8s/statefulset.yaml @@ -0,0 +1,111 @@ +# StatefulSet, not Deployment, and replicas: 1 -- both deliberate. See the comments. +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: codebaseqa-api + labels: + app: codebaseqa-api +spec: + serviceName: codebaseqa-api + # --------------------------------------------------------------------------------- + # WHY replicas: 1, AND WHY THAT IS NOT A TODO + # + # This application cannot currently run a second replica, for two reasons that are + # properties of the code and not of this manifest: + # + # 1. SQLite. DATABASE_URL is a file on the volume. A ReadWriteOnce PVC attaches to + # one node, and concurrent writers to one SQLite file across processes is exactly + # the configuration sqlite.org/howtocorrupt.html warns about. + # 2. ChromaDB is embedded. chroma_store.py holds a process-local persistent client + # over the same directory, so two pods would be two independent writers to one + # on-disk index. + # + # Startup itself is now safe to run concurrently -- the ALTER TABLE migrations were + # made idempotent rather than check-then-act, so a second replica no longer dies on + # "duplicate column name". But safe startup is not the same as safe operation. + # + # Scaling this past 1 means moving Postgres-ward and to a server-mode vector store. + # Until then, this number is a correct description of the system. + # --------------------------------------------------------------------------------- + replicas: 1 + selector: + matchLabels: + app: codebaseqa-api + template: + metadata: + labels: + app: codebaseqa-api + spec: + securityContext: + # The image creates uid 10001 and sets USER; runAsNonRoot makes the kubelet + # refuse to start the pod if that ever regresses to root. + runAsNonRoot: true + runAsUser: 10001 + runAsGroup: 10001 + # fsGroup is what makes the mounted PVC writable by the non-root user: the + # kubelet chowns the volume to this GID on mount. Without it the pod starts and + # then fails on the first SQLite write, which is a much worse failure than not + # starting. + fsGroup: 10001 + containers: + - name: api + image: codebaseqa-api:kind + # Never for a locally built tag: with IfNotPresent/Always, kind would try the + # registry and fail on an image that only exists in the node's containerd. + imagePullPolicy: Never + ports: + - name: http + containerPort: 8000 + envFrom: + - configMapRef: + name: codebaseqa-config + - secretRef: + name: codebaseqa-secrets + volumeMounts: + - name: data + mountPath: /app/data + # /api/health is intentionally NOT the readiness probe. + # + # It reports "degraded" whenever any dependency is unreachable -- including the + # LLM provider, which is a third party. Gating readiness on it would take the + # pod out of service because OpenAI had a bad minute, and in this cluster + # (no key configured) it would never become ready at all. + # + # Readiness asks the narrower question the kubelet actually needs: is this + # process serving HTTP? /api/platform/config touches only the local database. + readinessProbe: + httpGet: + path: /api/platform/config + port: http + initialDelaySeconds: 5 + periodSeconds: 5 + timeoutSeconds: 3 + failureThreshold: 6 + livenessProbe: + httpGet: + path: /api/platform/config + port: http + # Generous: startup runs migrations, the stuck-index reaper and Chroma + # initialization. Restarting mid-migration is worse than waiting. + initialDelaySeconds: 30 + periodSeconds: 20 + timeoutSeconds: 5 + failureThreshold: 3 + resources: + # Chroma plus the tree-sitter grammars are the floor here; 256Mi OOM-kills + # during indexing. + requests: + cpu: "100m" + memory: "512Mi" + limits: + memory: "2Gi" + volumeClaimTemplates: + - metadata: + name: data + spec: + # ReadWriteOnce is the honest access mode: it matches the single-writer + # constraint above rather than implying this could be shared. + accessModes: ["ReadWriteOnce"] + resources: + requests: + storage: 5Gi