4 Commits

Author	SHA1	Message	Date
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	76cadca8e3	fix: Kubernetes serialises an empty item list as null, not [] ... The drift check failed on every tick with: TypeError: 'NoneType' object is not iterable `resp.json().get("items", [])` cannot defend against this. The key IS present, with the value null, so the default never fires. An empty PartialObjectMetadataList comes back as `"items": null`, which is exactly what happens once the release selector matches nothing — the normal state of a cluster with no tenant releases provisioned yet. `or []` handles both shapes. The tests could not have caught this: all of them passed a real empty list, which is the one shape that works. The new test sends `"items": null` as the API server actually sends it, and is control-tested — restoring the old expression fails it with the same TypeError seen in production. Worth recording why this survived review. The API read was verified against a live cluster before shipping, but that cluster had 25 matching release secrets, so the empty case never ran. A measurement on real data proved the fast path worked and said nothing about the path taken when there is no data.	2026-07-20 15:17:51 +00:00
Nguyen Minh Phuc	4193a18cae	reconciler: read release secrets directly instead of shelling helm ... helm's list gunzips every release payload to build its table. The only fields either caller reads are name and namespace: reconciler/main.py live = {(r.name, r.namespace) for r in releases} worker/handlers.py releases = {r.name for r in await ...list_releases()} Both live in the release secret's labels and metadata, so nothing needs decompressing. Measured from a pod on this cluster: 21ms and 31KB, against helm's 4392ms. That is the whole fix for a drift check that was timing out at 330s every tick and OOMKilling the container at 256Mi. The cause of both was decompressing 96 releases to extract two strings each. It also means the container no longer cares that a cluster policy mutates its CPU request to 0 — at 21ms there is nothing left to starve. Three details carry the correctness, each with a test: - PartialObjectMetadataList in the Accept header asks for metadata only. Without it every release's gzipped manifest crosses the wire to be thrown away, which is the cost this exists to avoid. - The status selector drops superseded revisions server-side: 96 release secrets here, 25 live. Failed and pending states are kept, matching what `helm list` shows, because a failed release does exist and calling it missing would have the reconciler re-provision on top of it. - helm writes one secret per revision, up to 10 per release here, so the newest version label wins. Control-tested with a string compare, which picks "9" over "10". Out of cluster there is no ServiceAccount, so it falls back to helm and the e2e suite and laptop runs are unchanged. An API error raises rather than falling back: the fallback is for a known-absent ServiceAccount, and quietly retrying through helm would swap a visible error for the timeout this removes. chart and app_version are empty on this path rather than wrong — they live only in the compressed payload, and nothing reads them.	2026-07-20 13:05:25 +00:00
Nguyen Minh Phuc	2356ac4ef3	reconciler: scope the drift check to svcforge's own releases ... check_drift diffs helm against the database in both directions, and the second one is `live - known` -> drift.orphan_release at ERROR. `live` was every release in the cluster, so argocd, longhorn, gitea and cert-manager were all reported as orphans svcforge is failing to account for, on every sweep. They are not orphans; they were never svcforge's to know about. install() now stamps app.kubernetes.io/managed-by=svcforge and list_releases() selects on it. Releases provisioned before this see one sweep as missing and get re-provisioned, which is safe by construction — `helm upgrade --install` against a deterministic release name — and that re-provision applies the label. This does NOT fix the timeout, and the docstring says so. Measured, not assumed: unscoped 23 releases in 4392ms, scoped to 0 in 3988ms. `--selector` is not pushed down as a server-side selector, so helm still fetches and decompresses every release secret and filters what it already parsed. Around 10%, not the order of magnitude the flag's shape suggests. The 330s timeouts need CPU for the container or a different read path. The two sides of the label are asserted against each other rather than a literal, so a rename that updates only one fails in tests instead of in production as a silently empty drift check. Control-tested: renaming the read side alone fails two of the three.	2026-07-20 12:53:16 +00:00