Files
svcforge/services/reconciler/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

411 lines
18 KiB
Python

"""The control loop.
Every other service in svcforge is edge-triggered: a tenant POSTs, a row appears, a worker
claims it. Edge-triggered systems are correct exactly as long as nothing is ever missed —
and things are missed. A worker is SIGKILLed holding a lease. An operator runs
`helm uninstall` by hand. A pod dies between the CAS and the enqueue. Nobody sends an event
for any of that, because the thing that would have sent it is the thing that died.
So: level-triggered. Every 60 seconds, compare the world to the database and enqueue what
is missing. The four checks below do not know or care what went wrong, or whether anything
did; they are the same code on the happy path and after an outage. That property is the
entire reason this service exists, and it is why each check is written as a *query for
work*, never as a reaction to an event.
Three rules hold the design together:
* **Singleton.** `replicas: 1`, `strategy: Recreate` in the chart. Two reconcilers
double-enqueue drift and race on TTL. There is no leader election here on purpose: the
correct lease for that lives in Postgres next to the data, not in a Redis lock, and
until there is a second replica to elect between, an election is a subsystem that can
only fail. One pod, and the `SvcforgeReconcilerStale` alert is what notices it is gone.
* **Each check is independent.** One failing check must not skip the other three. A helm
binary that cannot reach the API server must not stop TTLs from expiring.
* **Enqueue, never act.** The reconciler diagnoses; workers treat. It writes task rows and
instance states, and never calls `helm install`. The one exception is reading — the drift
check runs `helm list`, because seeing reality is the job.
"""
from __future__ import annotations
import asyncio
import contextlib
import signal
from collections.abc import Awaitable, Callable
from dataclasses import dataclass
import typer
from svcforge_core.adapters.clock import Clock, SystemClock
from svcforge_core.adapters.helm import HelmProvisioner, Provisioner
from svcforge_core.adapters.notify import LogNotifier, Notifier
from svcforge_core.domain.catalog import load_catalog
from svcforge_core.domain.models import CatalogEntry
from svcforge_core.domain.windows import BadWindow, parse_window, schedule_upgrade_at
from svcforge_core.obs import (
INSTANCES,
QUEUE_DEPTH,
RECONCILER_LAST_TICK,
get_logger,
setup,
start_metrics_server,
tracer,
)
from svcforge_core.repo.db import DictPool, make_pool
from svcforge_core.repo.instances import InstanceRepo
from svcforge_core.repo.reconcile import ReconcileRepo
from svcforge_core.repo.tasks import TaskRepo
from svcforge_core.settings import Settings, load_settings
log = get_logger("svcforge.reconciler")
# The chart's PodMonitor scrapes the port named `metrics` on 9000. Keep them in step.
DEFAULT_METRICS_PORT = 9000
@dataclass(frozen=True)
class ReconcilerDeps:
"""Everything a check is allowed to touch. Built once in main(), passed down.
Same shape as `WorkerDeps` for the same reason: the checks take `deps` instead of
reaching for globals, so the integration tests below run every check against a real
Postgres and a `FakeProvisioner` without a cluster anywhere in sight.
"""
pool: DictPool
instances: InstanceRepo
tasks: TaskRepo
reconcile: ReconcileRepo
provisioner: Provisioner
notifier: Notifier
clock: Clock
catalog: dict[str, CatalogEntry]
settings: Settings
# Whose instances go first in the day-2 work list. Eating your own dog food is enforced
# by an `order by` rather than a policy document — see InstanceRepo.list_upgradable.
own_team: str
# A config value, not a scheduler. Leave it at 1 until 1 is too slow.
max_in_flight: int
# --- The four checks ---------------------------------------------------------------------
async def check_drift(deps: ReconcilerDeps) -> None:
"""`helm list -A -o json` versus what the database believes.
This is the only check that looks outside Postgres, and the only one that can catch the
failure nothing else can: someone ran `helm uninstall` by hand, or a node was drained
and the release never came back. The DB still says `ready` and still hands the tenant an
endpoint that resolves to nothing.
Two directions, two very different answers:
* **Release gone, DB says `ready`** -> re-enqueue provision. Safe, because provisioning
is `helm upgrade --install` against a deterministic release name: converging on
desired state, not a blind re-install.
* **Release exists, DB knows nothing** -> log at error with release and namespace, and
stop. **Never delete in v1.** The reconciler's view of "the DB knows nothing" is one
query against one database; the release might belong to another team, another tool,
or a migration half-finished. Deleting on that evidence is how an automated system
takes down production faster than any human could. A human reads the log and decides.
"""
with tracer().start_as_current_span("helm.list"):
releases = await deps.provisioner.list_releases()
live = {(r.name, r.namespace) for r in releases}
for inst in await deps.reconcile.ready_instances():
if (inst.release_name, inst.namespace) in live:
continue
reason = f"drift: helm release {inst.release_name} missing from namespace {inst.namespace}"
task_id = await deps.reconcile.enqueue_reprovision(inst.id, reason)
if task_id is None:
continue # already being dealt with, or the row moved under us
log.warning(
"drift.release_missing",
instance_id=str(inst.id),
team=inst.team,
release=inst.release_name,
namespace=inst.namespace,
task_id=task_id,
)
try:
await deps.notifier.send(
"drift.release_missing",
f"re-provisioning {inst.id}: release {inst.release_name} vanished",
{"instance_id": str(inst.id), "team": inst.team},
)
except Exception:
# The task is already committed; the notification is a courtesy. A webhook
# timing out must not abandon the rest of the sweep — the instances after this
# one in the loop have the same problem and nobody else is coming to find them.
log.exception("notify.failed", instance_id=str(inst.id))
known = await deps.reconcile.known_releases()
for name, namespace in sorted(live - known):
# error, not warning: this is a resource nobody is billing for and nobody owns.
# It will sit here every 60s until a human deletes it or adopts it. That is the
# intended pressure.
log.error("drift.orphan_release", release=name, namespace=namespace, action="none (v1 never deletes)")
async def check_lease_expiry(deps: ReconcilerDeps) -> None:
"""Tasks whose worker died -> back to `queued`.
A lease. No lock survives a power cut: a worker SIGKILLed mid-provision
leaves `state='running'` with `locked_by` set and nobody running it, and no amount of
cleanup code in the worker helps, because the worker is the part that died. `locked_at`
plus a timeout is the only thing that recovers the row, which is why `locked_at` exists.
The 5-minute default must exceed the longest a healthy task can hold a lease, or the
reconciler hands a still-running provision to a second worker. Handlers are idempotent,
so that is survivable, though it still costs a duplicated helm run — which is why
`lease_seconds` sits above helm's `--timeout`.
"""
freed = await deps.tasks.reset_expired_leases(deps.settings.lease_seconds)
if freed:
log.warning("lease.expired", tasks_freed=freed, lease_seconds=deps.settings.lease_seconds)
async def check_ttl(deps: ReconcilerDeps) -> None:
"""Expired instances -> `deleting`, plus a deprovision task.
The line item that stops a demo cluster from becoming a permanent cloud bill. Also the
sweep the API's DELETE route depends on: it CASes to `deleting` and enqueues in two
statements, and a crash in between lands here on the next tick.
Idempotent by construction — the work list excludes anything that already has a queued
or running deprovision, and the CAS and the insert share one transaction. Without that
guard, a deprovision that takes longer than 60 seconds gets a second task on the next
tick, and a third on the tick after.
"""
for inst in await deps.reconcile.due_for_deprovision():
task_id = await deps.reconcile.enqueue_deprovision(inst.id)
if task_id is None:
continue
log.info(
"ttl.expired",
instance_id=str(inst.id),
team=inst.team,
expires_at=inst.expires_at.isoformat() if inst.expires_at else None,
previous_state=inst.state.value,
task_id=task_id,
)
async def check_version_drift(deps: ReconcilerDeps) -> None:
"""The day-2 rollout: the work-list query, one service type at a time.
Everything that makes this safe is somewhere else, which is the point:
* `list_upgradable` limits to `max_in_flight` and returns nothing while
`rollout_state='halted'`, so a bad chart stops after one tenant.
* `schedule_upgrade_at` turns the tenant's maintenance window into a `run_after`; the
queue does the waiting, in `where run_after <= now()`. There is no scheduler here and
there must not be one — a task parked in Postgres until 03:00 Sunday survives a
reconciler restart, and an in-memory timer does not.
* `security: true` in the catalog bypasses the window. A CVE with a public exploit does
not wait until Sunday.
A bad window spec is this instance's problem, not the fleet's: log it and move to the
next one. Failing the whole check would let one tenant's typo freeze everyone's
security rollout.
"""
now = deps.clock.now()
for service_type, entry in deps.catalog.items():
candidates = await deps.instances.list_upgradable(
service_type=service_type,
catalog_version=entry.chart_version,
own_team=deps.own_team,
max_in_flight=deps.max_in_flight,
)
for candidate in candidates:
inst = candidate.instance
try:
window = parse_window(candidate.maintenance_window)
except BadWindow:
log.exception(
"upgrade.bad_window",
instance_id=str(inst.id),
team=inst.team,
maintenance_window=candidate.maintenance_window,
)
continue
run_after = schedule_upgrade_at(window, security=entry.security, now=now)
task_id = await deps.reconcile.enqueue_upgrade(inst.id, run_after)
if task_id is None:
continue # already queued or running; this is the max_in_flight guard
log.info(
"upgrade.scheduled",
instance_id=str(inst.id),
team=inst.team,
service_type=service_type,
from_version=inst.chart_version,
to_version=entry.chart_version,
run_after=run_after.isoformat(),
security=entry.security,
task_id=task_id,
)
CHECKS: dict[str, Callable[[ReconcilerDeps], Awaitable[None]]] = {
"drift": check_drift,
"lease_expiry": check_lease_expiry,
"ttl": check_ttl,
"version_drift": check_version_drift,
}
# --- The tick ----------------------------------------------------------------------------
async def tick(deps: ReconcilerDeps) -> None:
"""One pass: all four checks, then the gauges, then the heartbeat.
Checks first, gauges second: `svcforge_queue_depth` is read straight after the checks
that add to the queue, so the value scraped is the value the tick left behind rather
than one from before its own work.
The heartbeat is set unconditionally, and that is deliberate. It answers "is the loop
running", not "is everything fine" — the checks have their own alerts. Gating it on
success would make `SvcforgeReconcilerStale` fire for a helm blip and mean two things
at once, and an alert that means two things gets muted.
The whole tick runs inside one span, which is a considered exception to "manual spans go
around helm calls only". That rule exists so the API does not hand-roll spans that
`opentelemetry-instrument` already creates for it. Nothing auto-instruments the
reconciler: without a span here it emits no traces at all, and — because
`inject_traceparent` serialises the *active* context — every task it enqueues would be
written with a null `traceparent` and be unjoinable to the tick that decided to create
it. One span per tick is what makes "why was this instance re-provisioned at 03:00?" a
question the traces can answer.
"""
with tracer().start_as_current_span("reconciler.tick"):
await _run_checks(deps)
RECONCILER_LAST_TICK.set(deps.clock.now().timestamp())
log.info("tick.done")
async def _run_checks(deps: ReconcilerDeps) -> None:
"""The four checks and the gauges. Split out so `tick` reads as span + heartbeat."""
for name, check in CHECKS.items():
try:
await check(deps)
except Exception: # the tick is the error boundary
# The swallow is the design. These four checks share nothing but a database
# handle, and the value of a level-triggered loop is that it keeps running: an
# unreachable cluster must not stop TTLs from expiring, and one tenant's broken
# window spec must not stop drift detection. This means "this check achieved
# nothing for 60 seconds", which the log says out loud. It never means "the
# reconciler stops".
log.exception("check.failed", check=name)
try:
QUEUE_DEPTH.set(await deps.reconcile.queue_depth())
counts = await deps.reconcile.instance_counts()
for state, n in counts.items():
INSTANCES.labels(state=state).set(n)
except Exception: # gauges are diagnostics; a failed read is not a failed tick
log.exception("gauges.failed")
async def _sleep_or_stop(stop: asyncio.Event, seconds: float) -> None:
"""Sleep, but wake immediately on SIGTERM. A 60s nap must not cost 60s of shutdown."""
with contextlib.suppress(TimeoutError):
await asyncio.wait_for(stop.wait(), timeout=seconds)
async def run_reconciler(deps: ReconcilerDeps, stop: asyncio.Event) -> None:
"""Tick, sleep, repeat, until told to stop.
Tick first, then sleep: a pod that has just been restarted should reconcile now, not in
sixty seconds. Fixed interval rather than a fixed period — a tick that overruns simply
delays the next one, instead of stacking a second tick on top of the first, which for a
singleton would be exactly the concurrent reconciler `replicas: 1` exists to prevent.
"""
while not stop.is_set():
await tick(deps)
await _sleep_or_stop(stop, deps.settings.reconcile_interval_s)
def build_deps(
pool: DictPool,
settings: Settings,
own_team: str,
max_in_flight: int,
) -> ReconcilerDeps:
"""Wire the real collaborators. The only place that names concrete classes."""
return ReconcilerDeps(
pool=pool,
instances=InstanceRepo(pool),
tasks=TaskRepo(pool),
reconcile=ReconcileRepo(pool),
provisioner=HelmProvisioner(helm_bin=settings.helm_bin, timeout_s=int(settings.helm_timeout_s)),
notifier=LogNotifier(),
clock=SystemClock(),
catalog=load_catalog(settings.catalog_path),
settings=settings,
own_team=own_team,
max_in_flight=max_in_flight,
)
async def _amain(once: bool, metrics_port: int, own_team: str, max_in_flight: int) -> None:
settings = load_settings()
setup("svcforge-reconciler", settings)
pool = make_pool(settings.pg_dsn.unicode_string(), settings.pool_min_size, settings.pool_max_size)
await pool.open(wait=True)
deps = build_deps(pool, settings, own_team, max_in_flight)
try:
if once:
# One pass and exit: the acceptance path, and how you drive a reconcile by hand
# from a shell. No metrics server — nothing would ever scrape it.
await tick(deps)
return
start_metrics_server(metrics_port)
stop = asyncio.Event()
loop = asyncio.get_running_loop()
for sig in (signal.SIGTERM, signal.SIGINT):
# add_signal_handler, NOT signal.signal. signal.signal fires the handler at an
# arbitrary bytecode boundary on the main thread and the loop does not notice
# until its next timer — which here is up to a full 60s tick away. This one is
# scheduled as an ordinary loop callback, so the `stop.wait()` above returns
# immediately.
loop.add_signal_handler(sig, stop.set)
await run_reconciler(deps, stop)
finally:
await pool.close()
app = typer.Typer(add_completion=False, help="svcforge reconciler: the control loop.")
@app.command()
def main(
once: bool = typer.Option(False, "--once", help="Run one tick and exit."),
metrics_port: int = typer.Option(
DEFAULT_METRICS_PORT, envvar="SVCFORGE_METRICS_PORT", help="Port for /metrics."
),
own_team: str = typer.Option(
"platform", envvar="SVCFORGE_OWN_TEAM", help="Team whose instances upgrade first."
),
max_in_flight: int = typer.Option(
1, envvar="SVCFORGE_MAX_IN_FLIGHT", min=1, help="Concurrent upgrades across the fleet."
),
) -> None:
"""Run the reconciler."""
# One asyncio.run, at the top, never nested. Everything below it is already async.
asyncio.run(_amain(once, metrics_port, own_team, max_in_flight))
if __name__ == "__main__":
app()