Files
svcforge/services/api/main.py
T
Nguyen Minh Phuc c76154aeaa
ci / lint (push) Successful in 34s
ci / unit (push) Successful in 1m41s
ci / types (push) Successful in 1m41s
ci / dockerfile (push) Successful in 18s
ci / security (push) Successful in 1m27s
ci / chart (push) Failing after 1m11s
ci / integration (push) Successful in 1m10s
ci / image (api) (push) Has been skipped
ci / image (reconciler) (push) Has been skipped
ci / image (worker) (push) Has been skipped
ci / bump (push) Has been skipped
review: fix 26 findings from a 4-agent audit
CORRECTNESS
- lost-lease race: complete()/fail() did not check ownership, so a worker whose
  lease expired could mark a task done while another worker was running it, or
  requeue a task someone else owned. Reproduced, fixed with a CAS on
  (state, locked_by), pinned by two regression tests.
- worker died on report failure: _run_one's docstring claimed no exception
  escapes the TaskGroup; fail()/complete() were outside the guarded block, so a
  DB blip cancelled every sibling provision on the pod.
- claim query used an INNER join, which could strand a just-claimed task and
  report 'queue empty'. LEFT join.
- InstanceRepo.set_error bypassed the state machine and had no callers. Deleted.
- handle_deprovision ignored its CAS result, so a wrong-state instance kept a
  dangling endpoint and got re-provisioned by the drift check 60s later.
- handle_verify re-notified on every retry: five pages for one halt.

DEPLOY-BREAKING
- the migration Job could never succeed: no Dockerfile copied migrations/, and
  migrate.py resolved the path relative to the source tree, which only works for
  an editable install. Added COPY + SVCFORGE_MIGRATIONS_DIR.
- ServiceMonitor selector did not match the Service: API metrics never scraped.
- SvcforgeReconcilerStale fired permanently from every pod, because the gauge is
  module-level and every service exports it as 0. Scoped to the reconciler job.
- SvcforgeTaskFailed latched forever on a monotonic counter. Now increase()[15m].
- the digest guard accepted the all-zeros placeholder.
- worker terminationGracePeriodSeconds was 60s against a 600s helm timeout.

DEAD CODE THAT SHOULD NOT HAVE BEEN
- adapters/k8s.py was never called, so tenant namespaces were never created and
  the first provision for a new team would fail. Wired into handle_provision.
- adapters/redis.py was never imported by any service. Rate limiting is now wired
  into the API, failing open.
- Settings.check_production() had no callers. Given an explicit environment and
  called from every entrypoint.

OBSERVABILITY
- the API never called obs.setup(): no JSON logs, no trace correlation, log_json
  silently inert.
- LogNotifier's structured fields were discarded by the stdlib->structlog bridge.
- bind_task_context cleared the 'service' binding for the life of every task.
- split tasks_failed into task_attempts_failed and tasks_dead_lettered.

SECURITY
- trivy correctly blocked the worker/reconciler images: helm 3.16.2 and kubectl
  1.31.2 carry CRITICAL Go stdlib CVEs. Bumped to helm 3.21.3 and kubectl 1.35.3,
  which also closes a four-minor skew against the v1.35.3 cluster.

TESTS THAT COULD NOT FAIL
- the concurrency cap test passed on a fully serial worker.
- the alert/metric cross-check asserted a hardcoded list instead of reading the
  chart, so it could not catch a rename on the chart side.
- fixed OTel tracer-provider pollution between test files.

DOCS
- ARCHITECTURE.md: mermaid diagrams, user stories, and the helm-vs-ArgoCD
  guarantee (verified with --dry-run=server).
- AGENTS.md + CLAUDE.md.
- prose sweep for back-and-forth phrasing across 19 files.
2026-07-18 12:13:49 +00:00

147 lines
6.6 KiB
Python

"""The app factory and its lifespan.
`create_app(settings)` is a factory, not a module-level `app = FastAPI()`, for one reason:
a test needs an app pointed at a throwaway Postgres, and an import-time app reads the real
environment at import time — before any fixture can say otherwise.
"""
from __future__ import annotations
import asyncio
from collections.abc import AsyncIterator
from contextlib import asynccontextmanager
from fastapi import FastAPI, HTTPException, Request
from fastapi.responses import JSONResponse
from jwt import PyJWKClient
from services.api.models import ErrorBody
from services.api.routes import health, instances
from svcforge_core import obs
from svcforge_core.adapters.redis import RateLimiter, make_redis
from svcforge_core.domain.catalog import load_catalog
from svcforge_core.repo.db import make_pool
from svcforge_core.settings import Settings, load_settings
log = obs.get_logger("svcforge.api")
@asynccontextmanager
async def lifespan(app: FastAPI) -> AsyncIterator[None]:
"""Open the pool, yield, close the pool.
A lifespan context, not the deprecated startup/shutdown event decorators: those cannot
express "this resource lives for exactly as long as the app", and give you no place to
put the teardown next to the setup. Closing the pool matters — an unclosed pool means
connections linger server-side after SIGTERM, and on a pooled Postgres with a small
connection budget a few rolling deploys exhaust it.
(The old decorator's name is spelled nowhere in this package on purpose: CI greps for
the literal string, and a comment quoting it fails the gate just as loudly as a call.)
"""
settings: Settings = app.state.settings
app.state.catalog = load_catalog(settings.catalog_path)
# Redis is optional by construction. `make_redis` returns None when no DSN is set, and
# every consumer treats None as "skip" — so a deployment without Redis loses rate
# limiting and keeps everything else. Built here rather than per request because a
# connection pool per request is a connection pool per request.
redis = make_redis(settings)
app.state.redis = redis
app.state.rate_limiter = (
RateLimiter(redis, limit=settings.rate_limit_per_minute, window_s=60) if redis is not None else None
)
pool = make_pool(str(settings.pg_dsn), settings.pool_min_size, settings.pool_max_size)
# wait=True fails NOW, loudly, if the DSN is wrong — instead of at the first request,
# as a PoolTimeout, in front of a user.
await pool.open(wait=True)
app.state.pool = pool
# The pool is open from here on, so everything below is inside the try: an exception
# in JWKS setup must still close it, or a crash-looping pod leaks a connection per
# restart until the database refuses new ones.
try:
if settings.jwks_url and not settings.auth_disabled:
client = PyJWKClient(settings.jwks_url, cache_keys=True, lifespan=300)
app.state.jwks_client = client
# Warm the cache off the loop so the first authenticated request does not pay
# a blocking urlopen. Best-effort: a slow identity provider must not stop the
# pod from starting — a cache miss later just costs one to_thread hop.
try:
await asyncio.to_thread(client.get_signing_keys)
except Exception: # deliberate catch-all: startup must not hinge on the IdP being up
log.warning("JWKS warm-up failed; keys will be fetched on first use", exc_info=True)
else:
app.state.jwks_client = None
yield
finally:
await pool.close()
if redis is not None:
await redis.aclose()
async def _http_exception_handler(request: Request, exc: Exception) -> JSONResponse:
"""Render HTTPException bodies as ErrorBody, so every error has one shape.
Handlers raise `detail={"code": ..., "message": ...}`; FastAPI's default would nest
that under `{"detail": {...}}`. Plain-string details (raised by FastAPI itself, e.g.
a 405) are wrapped so clients never have to branch on the body's type.
"""
assert isinstance(exc, HTTPException) # noqa: S101 - registered only for HTTPException
# Widened to object deliberately. Starlette types `detail` as str, but FastAPI passes
# through whatever a handler raised — our handlers raise dicts. Narrowing off the
# declared type would make mypy call the dict branch unreachable and delete it.
detail: object = exc.detail
if isinstance(detail, dict) and "code" in detail and "message" in detail:
body = ErrorBody(code=str(detail["code"]), message=str(detail["message"]))
else:
body = ErrorBody(code=f"http_{exc.status_code}", message=str(detail))
return JSONResponse(status_code=exc.status_code, content=body.model_dump(), headers=exc.headers)
def create_app(settings: Settings | None = None) -> FastAPI:
"""App factory: lifespan, routers, exception handler, /metrics."""
settings = settings or load_settings()
# FIRST, before any router is built and before any logger is bound. Without this the
# API is the one service of three that never configures structlog: its lines go out
# through logging.lastResort as bare text on stderr with no service, no trace_id and
# no JSON envelope — a parse failure in the collector, and unattributable in Loki.
# `settings.log_json` was silently inert here for the same reason.
obs.setup("svcforge-api", settings)
# Refuse the dev escape hatches when SVCFORGE_ENVIRONMENT says this is not a laptop.
# Called unconditionally and early: a check that only runs from a branch someone
# remembered to write is a check that does not run.
settings.check_production()
app = FastAPI(
title="svcforge",
version="0.1.0",
summary="X-as-a-Service control plane",
lifespan=lifespan,
)
app.state.settings = settings
# /metrics is a normal route on health.router, not an app.mount — see health.metrics
# for why the mount does not actually serve a bare /metrics.
app.include_router(health.router)
app.include_router(instances.router)
app.add_exception_handler(HTTPException, _http_exception_handler)
return app
def app() -> FastAPI:
"""Entry point for `uvicorn services.api.main:app --factory`."""
return create_app()
# There is deliberately no `if __name__ == "__main__"` here. `services/api/__main__.py` is
# the single entrypoint, and the image's ENTRYPOINT uses it. A second one in this module
# drifted from it — different log_level, different access_log — so `python -m services.api`
# and `python services/api/main.py` started the same app two different ways.