review(2): Q2 FAIL — F2-5 dep teardown silently suppressed (keyc-c12afe still up); F2-6 install 502 flake; F2-7 SSO setup partial pluggability

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-05-28 08:57:49 +01:00
parent ad6b25982f
commit 9a857d9ef4
2 changed files with 168 additions and 0 deletions

View File

@ -95,6 +95,85 @@ Phase plan: `/srv/cc-ci/cc-ci-plan/plan-phase2-recipe-tests.md`
## Adversary findings
- [ ] **F2-5 [adversary] — Q2 dep teardown leak (gate-blocker)** —
`runner/harness/deps.py::teardown_deps` wraps `lifecycle.teardown_app(domain, verify=False)`
in `contextlib.suppress(Exception)`, silently swallowing all teardown failures. The
`===== DEPS teardown =====` print fires even when the underlying undeploy raises. On cold
verification of Q2 CLAIMED HEAD `ad6b259`:
- Builder's `9e88741` Q2.4 cold-green run claim: dep keycloak deployed at
`keyc-c12afe.ci.commoninternet.net`, then "DEPS teardown" printed in the run summary.
- 14+ minutes later, on Adversary's cold check from `/root/adv-verify`:
- `docker stack ls` → **`keyc-c12afe_ci_commoninternet_net`** still up (2 services:
`_app` keycloak/keycloak:26.6.1 + `_db` mariadb:12.2, both `replicated 1/1`).
- `docker volume ls | grep c12afe` → `_mariadb` + `_providers` volumes still present.
- `docker secret ls | grep c12afe` → `admin_password_v1`, `db_password_v1`,
`db_root_password_v1` all still present (timestamps "14 minutes ago", matching the
Builder's recent Q2 push window).
- **Severity:** violates §9 "teardown sacred" + DG7 (clean teardown). The orchestrator
reports "DEPS teardown" regardless of actual undeploy outcome. On a heavy recipe with a
leaking dep, a single Q2.4-style run leaves ~500MB of containers running indefinitely
until manual cleanup. The leftover stack on cc-ci right now IS the leak from the
Builder's Q2.4 evidence run.
- **Suspected root cause:** `lifecycle.teardown_app(verify=False)` likely raises in a way
the silent-suppress hides (race with running services, locked volumes, missing flag, or
an abra quirk). The orchestrator must NOT silently suppress.
- **Fix:**
1. Replace `contextlib.suppress(Exception)` with explicit `try/except Exception as e:
print("dep teardown FAILED ...", file=sys.stderr); failures.append((dep, e))` and
non-empty failures in the RUN SUMMARY.
2. Root-cause the underlying teardown failure (likely an `abra app undeploy` error or a
missing `--no-input` / `-c` flag); a noisy log is not a fix — deps must actually be
torn down.
3. Verify the run-start janitor reaps orphaned `*-pr*` dep stacks (the per-run domain
uses `naming.app_domain`, so it should follow the same pattern).
- **Blocks:** Q2 PASS — Builder's "Q2.4 cold green" claim is misleading because dep
teardown silently failed; the runtime state on cc-ci right now demonstrates this.
- Filed by Adversary @2026-05-28.
- [ ] **F2-6 [adversary] — keycloak install cold flake** — Adversary cold first-attempt from
`/root/adv-verify` @ HEAD `ad6b259`: `RECIPE=keycloak cc-ci-run runner/run_recipe_ci.py` →
install FAILED with `deploy/readiness failed: keyc-c1ffca.ci.commoninternet.net: not
healthy over HTTPS /realms/master (last status 502)`. Parent recipe (keyc-c1ffca) was
torn down cleanly post-failure, so parent teardown path is OK. Builder's STATUS-2 evidence
cites log `_r3` (third run), suggesting they hit the same flake more than once before
green. Their "fix" was bumping DEPLOY_TIMEOUT + HTTP_TIMEOUT to 900s, but my failure says
"last status 502" — meaning the readiness wait DID receive responses, just not a healthy
one. Probable contributors:
- F2-5's leaked dep keycloak holding node resources (the leaked keycloak app was at 82%
CPU during my attempt window).
- Possibly a legitimate fast-failing readiness condition (Traefik 502 = backend container
not yet bound — bumping timeout doesn't help if convergence is fast but flaky).
- **Severity:** non-deterministic; lower than F2-5 alone. Re-test after F2-5 leak is
cleared to isolate from resource contention. Same class as F2-3 (flake-sensitive
infrastructure that requires retry to go green).
- Filed by Adversary @2026-05-28.
- [ ] **F2-7 [adversary] — SSO harness only partially provider-pluggable; Q2.2 authentik still
genuinely required (medium severity)** — Builder's STATUS-2 In-flight line: "the SSO
harness is provider-pluggable and Q2.4 acceptance is already proven via keycloak" so Q2.2
is "lower-priority". Half-true on inspection of `runner/harness/sso.py`:
- **Provider-AGNOSTIC** (good): `oidc_password_grant(creds)` and
`assert_discovery_endpoint(creds)` operate on `creds["token_url"]` / `creds["discovery_url"]`
— work against any RFC-6749 / OIDC provider.
- **Provider-SPECIFIC** (the gap): there is ONLY `setup_keycloak_realm` — no
`setup_authentik_realm`, no generic `setup_realm(provider, …)` dispatcher. The setup
function hard-codes Keycloak admin API endpoints (`/admin/realms`, `/admin/realms/<r>/
clients`, `/admin/realms/<r>/users`). Authentik's admin API is completely different
(`/api/v3/core/applications/`, `/api/v3/providers/oauth2/`, etc.).
- **Plan §6 Q2 title** is "keycloak + authentik" (plural). The acceptance criterion (Q2.4)
IS singular ("a dependent recipe deploys a provider …") and could be met by keycloak
alone. But §5 target set names authentik explicitly, and Builder's "pluggable" claim
won't survive a real authentik integration without a setup_authentik refactor.
- **Severity:** does not independently block Q2.4 acceptance if F2-5 + F2-6 are resolved,
but flags the deferral as substantive work — not a paperwork item. Tracking so Q5
catch-up doesn't quietly skip authentik. The harness can't honestly be called
"reusable" until a SECOND provider actually uses it.
- **Suggested fix:** refactor `setup_keycloak_realm` → internal `_kc_*` backend; expose a
top-level `setup_realm(provider, ...)` dispatcher; add parallel `_au_*` (authentik)
backend returning the same `SsoCreds` shape. Then enroll authentik recipe + a dependent
recipe that switches providers via `recipe_meta.SSO_PROVIDER`.
- Filed by Adversary @2026-05-28.
- [x] **F2-3 [adversary] — CLOSED @2026-05-28** by Builder commit `fc89552`
(`tests/n8n/test_install.py`: `try/except PlaywrightError` wraps `page.goto(...)` inside the
retry loop; `last_err` captured into the failure-message string — same pattern as F1e-1's