4 Commits

Author	SHA1	Message	Date
Nguyen Minh Phuc	66eb6cb0ee	refactor: converge the patterns multiple authors left divergent ... The codebase was written by several agents and had the same concept done more than one way. This makes it read as one voice, with no behaviour change. Dedup, each to a single canonical form: - INSTANCE_COLUMNS: the 13-column instances SELECT list existed as _COLUMNS in instances.py and reconcile.py (byte-identical) and inlined a third time in the worker. One exported constant now. - Settings.runtime_dsn: the three entrypoints each chose between str(pg_dsn) and pg_dsn.unicode_string(). One property. - yaml_tempfile: helm._ValuesFile and k8s._ManifestFile were the same write-yaml-to-a-temp-dir context manager. One helper in adapters/tempyaml.py. - services/_runtime.py: sleep_or_stop and install_stop_signals were copied between the worker and reconciler loops. One module, so shutdown behaviour cannot drift between them. - k8s.ensure_namespace used MANAGED_BY_LABEL/VALUE from helm.py instead of a hardcoded literal, so the managed-by label has one definition. - SvcforgeError is now the root of every svcforge exception (CatalogError, IllegalTransition, BadWindow, HandlerError), keeping each stdlib base in the MRO, so `except SvcforgeError` means what errors.py says it does. - ERROR_MAX_CHARS replaces the repeated `[-2000:]` truncation feeding the same error columns. - the reconciler reads settings.metrics_port like the worker, dropping its duplicate DEFAULT_METRICS_PORT and redundant --metrics-port option; the SVCFORGE_METRICS_PORT env override still applies through pydantic. Two smaller correctness/consistency fixes: - RateLimitResult.retry_after_s computed its delta against datetime.now(UTC) while the limiter runs on an injectable clock, so it was meaningless under a FakeClock and drifted by request latency in production. It now carries a checked_at from the same clock as reset_at. - handle_provision's notifier.send is wrapped like the reconciler's: a flaky webhook after the READY CAS would fail the task, and the retry would hit the READY early-return and drop the notification, turning a good provision into a failed one.	2026-07-21 01:46:54 +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 ... 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 ... 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