Commit Graph

4 Commits

Author SHA1 Message Date
Nguyen Minh Phuc bb8b14ef03 api: make the OpenAPI spec usable as documentation
ci / lint (push) Failing after 56s
ci / types (push) Has been skipped
ci / unit (push) Has been skipped
ci / integration (push) Has been skipped
ci / security (push) Has been skipped
ci / dockerfile (push) Has been skipped
ci / chart (push) Has been skipped
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
FastAPI already served /docs, /redoc and /openapi.json, and the ingress already
passed them through — the mechanism was there, the content was not. The schema
alone cannot tell a caller the three things they most need to know, and the
route docstrings explain implementation reasoning to a maintainer rather than
usage to a consumer.

Added to the spec itself, so it travels with the API rather than living in a
README the caller does not have:

  - An app description covering bearer auth, that every write is 202 + poll,
    the instance lifecycle, and the uniform {"code", "message"} error body.
  - Tag descriptions for `instances` and `ops`.
  - Field descriptions and worked examples on CreateInstanceRequest,
    InstanceResponse and ErrorBody, so /docs shows a valid payload instead of
    leaving callers to infer one.

The bearer scheme was already exposed via HTTPBearer, which is what makes the
Authorize button in /docs work; there is now a test asserting it stays, along
with the description, the tags and the request example. Docs that are not
tested rot silently, and this is the artifact other teams integrate against.

README documents the three URLs and how to generate a client from the spec.
2026-07-21 05:33:44 +00:00
Nguyen Minh Phuc d64c3c9f39 fix: three correctness bugs found in review + a dropped-log
fail() blanket-marked the instance `failed` for every dead-lettered task kind.
Only provision is correct. The others each left the instance in a state the
UPDATE then corrupted:
  - deprovision: `deleting` -> `failed` stranded the instance, because
    due_for_deprovision only re-selects ready/deleting, leaking the release
    reconcile.py promises to reclaim.
  - upgrade: helm --atomic rolled back, so the instance was still `ready` and
    serving the old version; `failed` mislabelled a healthy service and dropped
    it off the upgrade work-list.
  - verify: handle_verify already halted the rollout; the instance was `ready`.
Now gated on kind == provision, with a regression test per kind (control-tested
against the blanket UPDATE, which fails all three).

The API exception handler was registered on fastapi.HTTPException, a subclass
of starlette's. Starlette matches handlers by walking type(exc).__mro__, so
framework-raised 404/405 never hit it and returned {"detail": ...} instead of
ErrorBody. Registered on the starlette parent, and added a
RequestValidationError handler so body-validation 422s share the shape too.
Tests assert the ErrorBody shape for framework 404, 405, and a forbidden field.

helm._list_releases_via_api built the httpx client with verify=<ca path>, which
loads the CA eagerly and raises OSError on a half-mounted ServiceAccount — an
error the except clause did not catch, crashing the reconciler tick as a bare
bug. Now the CA is checked for readability alongside the token, and a missing
one means "not in-cluster" and falls back to helm. Also documents the no-limit
pagination invariant and pins it with a test.

redis.py logged through stdlib logging with extra={"team": team}, which the
structlog bridge drops on the floor — the trap notify.py documents. Switched to
a bound logger with team as a kwarg.
2026-07-21 01:46:54 +00:00
Nguyen Minh Phuc c76154aeaa review: fix 26 findings from a 4-agent audit
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
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
Nguyen Minh Phuc 50c2fe2a1e svcforge: reference implementation
ci / lint (push) Successful in 1m19s
ci / unit (push) Failing after 1m2s
ci / integration (push) Has been skipped
ci / types (push) Successful in 1m37s
ci / security (push) Failing after 38s
ci / dockerfile (push) Successful in 14s
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
Complete working build of the system learn-python/ teaches.
164 tests, mypy --strict clean, domain coverage 99%.
2026-07-17 10:44:54 +00:00