Files
svcforge/README.md
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

127 lines
6.9 KiB
Markdown

# svcforge — reference implementation
A complete, working, verified build of the system `../learn-python/` teaches you to build.
## Read this part first
**This folder can ruin the course, and it will if you let it.**
`learn-python/AGENTS.md` opens with a rule: *"Do not write his implementation code for him.
`domain/`, `repo/`, the claim loop, the handlers — those are the course."* This repo is
exactly that code. You asked for it deliberately, and the rule allows you to overrule it —
but the reason for the rule did not go away when you did.
Module 4 is five evenings of getting the claim query wrong, and those five evenings are the
learning. The evening you spend watching two workers grab the same task is the evening
`FOR UPDATE SKIP LOCKED` stops being a phrase and becomes something you understand.
Reading `repo/tasks.py` here takes ninety seconds, teaches you close to nothing, and feels
exactly like learning. That feeling is the trap.
So:
| Use it like this | Not like this |
|---|---|
| Attempt the module. Get stuck. Stay stuck 30 minutes. **Then** diff your version against this one. | Open this first "just to see the shape". |
| Steal the scaffolding — `pyproject.toml`, Dockerfiles, `ci.yaml`, the chart. Nothing is learned by fighting hatchling. | Copy `domain/`, `repo/`, `handlers.py`, or the claim loop. |
| Read the **comments**. They explain *why*, which is the part that transfers. | Read the code. It is the part that doesn't. |
| Use it to check an answer you already produced. | Use it to produce an answer. |
The scaffolding is where struggling teaches you nothing. The domain and the queue are the
whole point. Know which one you're reading.
## What is actually verified
Everything below was executed, not asserted:
| Claim | How it was proven |
|---|---|
| The claim query never double-claims | 50 concurrent workers, 50 tasks, real Postgres. Each claimed exactly once. |
| The transaction story is real | Instance + task roll back together on an abort. |
| SIGTERM drains in flight | Worker finishes a 2s provision after stop is set, exits 0. |
| Handlers are idempotent | Re-running `handle_provision` installs once, not twice. |
| helm timeouts kill the process **group** | A negative control with `proc.kill()` leaks two `sleep 300`s; the real `_run` leaves zero. |
| The state machine is enforced in SQL too | Guard test fails when the guard is removed (control-tested). |
| Redis degrades safely | Rate limit fails open, cache falls through, platform stays up with Redis dead. |
| The images ship a wheel, not an editable | Negative control proved `uv sync` alone ships `/app/libs`; `--no-editable` fixed it. |
| The chart is valid | `helm lint`, `helm template`, `hadolint` — all clean. |
Numbers from a real drain (400 tasks, local Postgres, FakeProvisioner) are in
[RUNBOOK.md](RUNBOOK.md#measured-numbers).
## Running it
**No Docker on this host, deliberately.** This box's `containerd` belongs to a Kubernetes
kubelet; installing `docker.io` would evict the runtime and take every pod with it. So the
integration tests take a DSN from the environment and only fall back to testcontainers
(the CI path) when it is absent:
```bash
sudo -u postgres psql -c "create role svcforge login password 'svcforge' superuser;"
sudo -u postgres psql -c "create database svcforge_test owner svcforge;"
export PATH="$HOME/.local/bin:$PATH"
export SVCFORGE_TEST_DSN="postgresql://svcforge:svcforge@127.0.0.1:5432/svcforge_test"
make dev # uv sync
make lint # ruff + ruff format + mypy --strict
uv run pytest -q -m "not e2e and not slow"
```
Each pytest process clones its own database, so concurrent runs don't truncate each other.
Redis tests marked `slow` hit real Upstash. **Mind the budget**: the free tier is 500K
commands/month = 0.19/sec sustained. The whole suite spends about 35.
```bash
set -a; . ~/.config/svcforge/secrets.env; set +a # never commit these
uv run pytest -q -m slow
uv run python -m scripts.redis_budget # projects month-end burn, exits 1 if over
```
## Where things live
| Module | Teaches | Read here |
|---|---|---|
| 0 | toolchain | `pyproject.toml`, `Makefile` |
| 1 | domain, pydantic, Protocol | `libs/svcforge_core/svcforge_core/domain/` |
| 2 | psycopg3, repos, migrations | `repo/{db,instances}.py`, `migrations/001_init.sql` |
| 3 | FastAPI, JWT, one transaction | `services/api/` |
| **4** | **the queue — the core** | **`repo/tasks.py`, `services/worker/`** |
| 5 | subprocess, timeouts, Protocols | `adapters/helm.py` (read `_run` twice) |
| 6 | day-2 fleet upgrades | `domain/windows.py`, `InstanceRepo.list_upgradable` |
| 7 | reconciler, obs | `services/reconciler/`, `obs.py` |
| 8 | packaging, CI, ArgoCD | `services/*/Dockerfile`, `.gitea/workflows/ci.yaml`, `deploy/` |
| 9 | chaos, load, CLI, runbook | `scripts/load.py`, `services/cli/`, `RUNBOOK.md` |
| 10 | Redis as a shortcut | `adapters/redis.py`, `scripts/redis_budget.py` |
## Where this deviates from the spec, and why
Honest list. Each was a real conflict, not a shortcut.
1. **`TaskRepo.enqueue` exists twice.** Module 2 specs `enqueue(conn, ...)`; Module 4 specs
`enqueue(instance_id, kind) -> int`. Both callers are real, so both exist:
`enqueue(conn, ...)` and `enqueue_standalone(...)`. Making `conn` optional would have
hidden the transaction question, which is the one thing that module is about.
2. **The claim query has a CTE wrapper.** The spec calls it byte-identical. The `update ...
for update skip locked` shape is untouched; an outer `select` joins `instances.team` so
the worker can bind `team` to its logs at claim time. The alternative was a second round
trip per task for a column the DB already had.
3. **The day-2 work-list query gained `service_type` and a halted check.** As printed it
compares every service type against one version, and never consults `catalog_versions`
despite the same module requiring halted → 0 rows.
4. **`Role` → `ClusterRole`.** The specced rules grant `create` on `namespaces`, which is
cluster-scoped. A namespaced Role cannot express it. Rules verbatim otherwise.
5. **DELETE is not one transaction.** `InstanceRepo.update_state` owns its connection, so
the CAS and the enqueue can't share one without reaching around the repo. Ordered for
the failure mode instead: CAS first, enqueue second — a crash between leaves `deleting`
with no task, which the reconciler sweeps up. The reverse would tear down a live service.
6. **Traceparent is `-03`, not the spec's `-01`.** Current SDKs set the random-trace-id bit
alongside sampled. The spec's acceptance line is stale.
7. **`make_redis` returns `Redis | None`.** "No Redis configured" has to be runnable.
## What was deliberately NOT built
Because Module 6 says so, and the restraint is the lesson: no `resize`, no `backup`/`restore`,
no `helm rollback` automation, no deprecation timers, no rollouts table, no pause/resume CLI.
A halted rollout is one column, cleared by hand with SQL.