From 1f9c816082218c3b2d8ddb05637162cb2ad55cf1 Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 12:24:59 +0300 Subject: [PATCH 01/17] feat(opal-server): gated /internal git-fetcher cache stats endpoint Add an off-by-default diagnostics endpoint so tests can observe the in-memory GitPolicyFetcher cache sizes (repo_locks/repos/repos_last_fetched) and process RSS that the upcoming memory-leak fix eliminates. - debug_stats.py: read-only git_fetcher_cache_stats() helper + a register_internal_stats_route() registrar that mounts GET /internal/git-fetcher-cache-stats only when enabled. - config.py: new OPAL_DEBUG_INTERNAL_STATS flag, default False. - server.py: register the route, gated by the flag, beside /healthcheck. No production behavior change when the flag is off (the default). Also ignore .claude/ so private planning artifacts are never committed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitignore | 3 ++ packages/opal-server/opal_server/config.py | 9 ++++ .../opal-server/opal_server/debug_stats.py | 41 +++++++++++++++++++ packages/opal-server/opal_server/server.py | 5 +++ .../tests/debug_stats_endpoint_test.py | 23 +++++++++++ .../opal_server/tests/debug_stats_test.py | 21 ++++++++++ 6 files changed, 102 insertions(+) create mode 100644 packages/opal-server/opal_server/debug_stats.py create mode 100644 packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py create mode 100644 packages/opal-server/opal_server/tests/debug_stats_test.py diff --git a/.gitignore b/.gitignore index 77faef34a..c3224ec1c 100644 --- a/.gitignore +++ b/.gitignore @@ -137,3 +137,6 @@ dmypy.json *.iml .DS_Store + +# Private Claude Code working artifacts (plans/specs) — never commit +.claude/ diff --git a/packages/opal-server/opal_server/config.py b/packages/opal-server/opal_server/config.py index 9faac9be4..aaf3d6f75 100644 --- a/packages/opal-server/opal_server/config.py +++ b/packages/opal-server/opal_server/config.py @@ -358,6 +358,15 @@ class OpalServerConfig(Confi): description="Set if OPAL server should enable tracing with datadog APM", ) + DEBUG_INTERNAL_STATS = confi.bool( + "DEBUG_INTERNAL_STATS", + False, + description=( + "Expose GET /internal/git-fetcher-cache-stats with in-memory cache " + "sizes and process RSS. For diagnostics/tests only; keep off in production." + ), + ) + SCOPES = confi.bool("SCOPES", default=False, description="Enable scopes") SCOPES_REPO_CLONES_SHARDS = confi.int( diff --git a/packages/opal-server/opal_server/debug_stats.py b/packages/opal-server/opal_server/debug_stats.py new file mode 100644 index 000000000..20ea6b341 --- /dev/null +++ b/packages/opal-server/opal_server/debug_stats.py @@ -0,0 +1,41 @@ +"""Read-only introspection of the git-fetcher in-memory caches. + +Used only by the off-by-default /internal stats endpoint so tests can +observe the cache growth that the memory-leak fix (PR2) eliminates. +""" +from pathlib import Path +from typing import Dict + +from fastapi import FastAPI +from opal_server.git_fetcher import GitPolicyFetcher + + +def _read_rss_kb() -> int: + """Resident set size of this process in kilobytes (Linux), else 0.""" + try: + for line in Path("/proc/self/status").read_text().splitlines(): + if line.startswith("VmRSS:"): + return int(line.split()[1]) + except (OSError, ValueError, IndexError): + return 0 + return 0 + + +def git_fetcher_cache_stats() -> Dict[str, int]: + """Sizes of the three process-global GitPolicyFetcher caches + RSS.""" + return { + "repo_locks": len(GitPolicyFetcher.repo_locks), + "repos": len(GitPolicyFetcher.repos), + "repos_last_fetched": len(GitPolicyFetcher.repos_last_fetched), + "rss_kb": _read_rss_kb(), + } + + +def register_internal_stats_route(app: FastAPI, enabled: bool) -> None: + """Mount GET /internal/git-fetcher-cache-stats only when enabled.""" + if not enabled: + return + + @app.get("/internal/git-fetcher-cache-stats", include_in_schema=False) + def _git_fetcher_cache_stats() -> Dict[str, int]: + return git_fetcher_cache_stats() diff --git a/packages/opal-server/opal_server/server.py b/packages/opal-server/opal_server/server.py index a846e5a80..2b88b3db8 100644 --- a/packages/opal-server/opal_server/server.py +++ b/packages/opal-server/opal_server/server.py @@ -26,6 +26,7 @@ ) from opal_server.config import opal_server_config from opal_server.data.api import init_data_updates_router +from opal_server.debug_stats import register_internal_stats_route from opal_server.data.data_update_publisher import DataUpdatePublisher from opal_server.loadlimiting import init_loadlimit_router from opal_server.policy.bundles.api import router as bundles_router @@ -293,6 +294,10 @@ async def redoc_html(req: Request) -> HTMLResponse: def healthcheck(): return {"status": "ok"} + register_internal_stats_route( + app, enabled=opal_server_config.DEBUG_INTERNAL_STATS + ) + return app def _configure_lifecycle_callbacks(self, app: FastAPI): diff --git a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py new file mode 100644 index 000000000..8ba0f678a --- /dev/null +++ b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py @@ -0,0 +1,23 @@ +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from opal_server.debug_stats import register_internal_stats_route + + +def _app_with_flag(enabled: bool) -> FastAPI: + app = FastAPI() + register_internal_stats_route(app, enabled=enabled) + return app + + +def test_endpoint_absent_when_disabled(): + client = TestClient(_app_with_flag(False)) + assert client.get("/internal/git-fetcher-cache-stats").status_code == 404 + + +def test_endpoint_present_when_enabled(): + client = TestClient(_app_with_flag(True)) + resp = client.get("/internal/git-fetcher-cache-stats") + assert resp.status_code == 200 + body = resp.json() + assert set(body) == {"repo_locks", "repos", "repos_last_fetched", "rss_kb"} diff --git a/packages/opal-server/opal_server/tests/debug_stats_test.py b/packages/opal-server/opal_server/tests/debug_stats_test.py new file mode 100644 index 000000000..b2d7935db --- /dev/null +++ b/packages/opal-server/opal_server/tests/debug_stats_test.py @@ -0,0 +1,21 @@ +from opal_server.config import opal_server_config +from opal_server.debug_stats import git_fetcher_cache_stats +from opal_server.git_fetcher import GitPolicyFetcher + + +def test_stats_report_dict_sizes(monkeypatch): + monkeypatch.setattr(GitPolicyFetcher, "repo_locks", {"a": object()}) + monkeypatch.setattr(GitPolicyFetcher, "repos", {"p1": object(), "p2": object()}) + monkeypatch.setattr(GitPolicyFetcher, "repos_last_fetched", {}) + + stats = git_fetcher_cache_stats() + + assert stats["repo_locks"] == 1 + assert stats["repos"] == 2 + assert stats["repos_last_fetched"] == 0 + assert isinstance(stats["rss_kb"], int) + assert stats["rss_kb"] >= 0 + + +def test_internal_stats_flag_defaults_off(): + assert opal_server_config.DEBUG_INTERNAL_STATS is False From c176ce9dbbf89bd92b70c3aad5ee486259868a4a Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 12:24:59 +0300 Subject: [PATCH 02/17] test(git-leak): add OPAL git leak/resilience test bed A self-contained docker-compose stack (opal-server x2 workers + Redis + Postgres broadcaster + Gitea) plus a pytest harness that reproduces, as tests that fail on master, the git-fetcher memory leak, the offline-repo hang, the slow serial boot, and the broadcaster-disconnect gap. These become the regression gates for the follow-up fixes. - seed/: idempotent Gitea seeding sidecar (N policy repos) + Dockerfile. - docker-compose.yml: 4-service stack, opal-server built from the repo's own docker/Dockerfile (server target), scopes on, Postgres broadcaster. - helpers.py / conftest.py: HTTP + infra helpers and stack fixtures. - test_leak.py / test_resilience.py / test_boot.py: the flagship tests. - README.md: how to run and expected fail-on-master behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/README.md | 33 +++++++ app-tests/git-leak/conftest.py | 44 +++++++++ app-tests/git-leak/docker-compose.yml | 103 ++++++++++++++++++++ app-tests/git-leak/helpers.py | 94 +++++++++++++++++++ app-tests/git-leak/seed/Dockerfile | 6 ++ app-tests/git-leak/seed/seed_gitea.py | 130 ++++++++++++++++++++++++++ app-tests/git-leak/test_boot.py | 37 ++++++++ app-tests/git-leak/test_leak.py | 56 +++++++++++ app-tests/git-leak/test_resilience.py | 54 +++++++++++ 9 files changed, 557 insertions(+) create mode 100644 app-tests/git-leak/README.md create mode 100644 app-tests/git-leak/conftest.py create mode 100644 app-tests/git-leak/docker-compose.yml create mode 100644 app-tests/git-leak/helpers.py create mode 100644 app-tests/git-leak/seed/Dockerfile create mode 100644 app-tests/git-leak/seed/seed_gitea.py create mode 100644 app-tests/git-leak/test_boot.py create mode 100644 app-tests/git-leak/test_leak.py create mode 100644 app-tests/git-leak/test_resilience.py diff --git a/app-tests/git-leak/README.md b/app-tests/git-leak/README.md new file mode 100644 index 000000000..33a291f86 --- /dev/null +++ b/app-tests/git-leak/README.md @@ -0,0 +1,33 @@ +# OPAL git-leak / resilience test bed + +Reproduces (as failing tests on `master`) the four issues fixed by PR2–PR5: +memory leak, offline-repo hang, slow serial boot, broadcaster no-reconnect. + +## Stack +- `opal_server` (2 workers, scopes on, Postgres broadcaster, built from `docker/Dockerfile`) +- `redis`, `postgres`, `gitea` (+ one-shot `gitea-admin` and `seed` sidecars) + +## Run +```bash +cd app-tests/git-leak +python -m pytest -v --boot-scopes=50 # full set +python -m pytest test_leak.py -v --boot-scopes=20 # just the leak gates +``` +Useful flags: `--boot-scopes=N` (any N), `--keep-stack` (skip teardown), +env `BOOT_TARGET_SECONDS=120` (tighten the boot gate). + +## Expected on master +All five flagship tests FAIL (except the boot test, which only fails when +`BOOT_TARGET_SECONDS` is set low). They become the regression gates for the fixes. + +## Requires +Docker + docker compose v2, plus host Python with `pytest pytest-timeout requests GitPython`. + +## Notes +- Auth is disabled in the stack: `OPAL_AUTH_PUBLIC_KEY` is left unset so the JWT + verifier is disabled and the harness can call scope routes without minting JWTs. + Local test bed only; never a production setting. +- The server runs 2 uvicorn workers with a Postgres broadcaster, mirroring a + realistic multi-worker deployment. The `GitPolicyFetcher` caches read by the + `/internal/git-fetcher-cache-stats` endpoint are per-process, so the harness + polls with generous timeouts to let the leader worker converge. diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py new file mode 100644 index 000000000..8c538aa06 --- /dev/null +++ b/app-tests/git-leak/conftest.py @@ -0,0 +1,44 @@ +import os + +import pytest + +from helpers import OpalServerClient, compose + + +def pytest_addoption(parser): + parser.addoption( + "--boot-scopes", + action="store", + default="50", + help="number of repos to seed/boot (default 50)", + ) + parser.addoption( + "--keep-stack", + action="store_true", + default=False, + help="do not tear the compose stack down after the run", + ) + + +@pytest.fixture(scope="session") +def repo_count(request) -> int: + return int(request.config.getoption("--boot-scopes")) + + +@pytest.fixture(scope="session") +def stack(request, repo_count): + os.environ["REPO_COUNT"] = str(repo_count) + # build + start infra; seed runs to completion then exits + compose("up", "-d", "--build") + # block until seeding sidecar has finished creating repos + compose("wait", "seed") + client = OpalServerClient() + client.wait_healthy() + yield client + if not request.config.getoption("--keep-stack"): + compose("down", "-v") + + +@pytest.fixture() +def opal(stack) -> OpalServerClient: + return stack diff --git a/app-tests/git-leak/docker-compose.yml b/app-tests/git-leak/docker-compose.yml new file mode 100644 index 000000000..8f9f474f9 --- /dev/null +++ b/app-tests/git-leak/docker-compose.yml @@ -0,0 +1,103 @@ +name: opal-git-leak-test + +services: + redis: + image: redis:7-alpine + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 2s + timeout: 3s + retries: 30 + + postgres: + image: postgres:16-alpine + environment: + POSTGRES_USER: opal + POSTGRES_PASSWORD: opal + POSTGRES_DB: opal + # not published to the host: only opal_server reaches it over the compose + # network, and bounce_postgres() uses `docker compose stop/start`. Publishing + # 5432 would collide with any Postgres already running on the host. + healthcheck: + test: ["CMD-SHELL", "pg_isready -U opal"] + interval: 2s + timeout: 3s + retries: 30 + + gitea: + image: gitea/gitea:1.21 + environment: + GITEA__security__INSTALL_LOCK: "true" + GITEA__server__ROOT_URL: "http://gitea:3000/" + GITEA__database__DB_TYPE: "sqlite3" + # not published to the host: only the seed sidecar and opal_server reach it + # over the compose network (via http://gitea:3000). Avoids host port clashes. + volumes: + - gitea-data:/data + healthcheck: + test: ["CMD-SHELL", "wget -qO- http://localhost:3000/api/v1/version || exit 1"] + interval: 3s + timeout: 5s + retries: 40 + + gitea-admin: + # creates the admin user once gitea is healthy + image: gitea/gitea:1.21 + depends_on: + gitea: + condition: service_healthy + user: git + entrypoint: ["/bin/sh", "-c"] + command: + - > + gitea admin user create --username opaladmin --password opaladmin + --email admin@example.com --admin --must-change-password=false + --config /data/gitea/conf/app.ini || true + volumes: + - gitea-data:/data + restart: "no" + + seed: + build: ./seed + depends_on: + gitea: + condition: service_healthy + gitea-admin: + condition: service_completed_successfully + environment: + GITEA_URL: "http://gitea:3000" + GITEA_ADMIN_USER: "opaladmin" + GITEA_ADMIN_PASSWORD: "opaladmin" + REPO_COUNT: "${REPO_COUNT:-50}" + volumes: + - seed-output:/seed-output + restart: "no" + + opal_server: + build: + context: ../.. + dockerfile: docker/Dockerfile + target: server + environment: + UVICORN_NUM_WORKERS: "2" + OPAL_SCOPES: "1" + OPAL_REDIS_URL: "redis://redis:6379" + OPAL_BROADCAST_URI: "postgres://opal:opal@postgres:5432/opal" + OPAL_BASE_DIR: "/opal" + OPAL_POLICY_REFRESH_INTERVAL: "0" + OPAL_DEBUG_INTERNAL_STATS: "1" + # OPAL_AUTH_PUBLIC_KEY is intentionally left unset: with no public key the + # JWT verifier is disabled, so the harness can call scope routes without + # minting JWTs. Local test bed only; never a production setting. + OPAL_LOG_FORMAT_INCLUDE_PID: "true" + ports: + - "7002:7002" + depends_on: + redis: + condition: service_healthy + postgres: + condition: service_healthy + +volumes: + gitea-data: + seed-output: diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py new file mode 100644 index 000000000..93871f194 --- /dev/null +++ b/app-tests/git-leak/helpers.py @@ -0,0 +1,94 @@ +"""HTTP + infra helpers for the git-leak test bed.""" +import subprocess +import time +from pathlib import Path +from typing import Dict, List + +import requests + +OPAL_URL = "http://localhost:7002" +GITEA_INTERNAL_URL = "http://gitea:3000" +GITEA_USER = "opaladmin" + +# the compose project lives next to this file; compose() runs from here +_COMPOSE_DIR = str(Path(__file__).resolve().parent) + + +class OpalServerClient: + def __init__(self, base_url: str = OPAL_URL): + self.base_url = base_url.rstrip("/") + + def wait_healthy(self, timeout: int = 180) -> None: + deadline = time.time() + timeout + last = None + while time.time() < deadline: + try: + if requests.get(f"{self.base_url}/healthcheck", timeout=5).status_code == 200: + return + except requests.RequestException as exc: + last = exc + time.sleep(2) + raise RuntimeError(f"opal-server not healthy in {timeout}s (last: {last})") + + def stats(self) -> Dict[str, int]: + resp = requests.get( + f"{self.base_url}/internal/git-fetcher-cache-stats", timeout=10 + ) + resp.raise_for_status() + return resp.json() + + def put_scope(self, scope_id: str, repo_url: str, branch: str = "main") -> None: + body = { + "scope_id": scope_id, + "policy": { + "source_type": "git", + "url": repo_url, + "auth": {"auth_type": "none"}, + "branch": branch, + "directories": ["."], + "extensions": [".rego", ".json"], + "manifest": ".manifest", + "poll_updates": False, + }, + "data": {"entries": []}, + } + # the scope router mounts at prefix="/scopes" with @router.put("") + resp = requests.put(f"{self.base_url}/scopes", json=body, timeout=30) + resp.raise_for_status() + + def delete_scope(self, scope_id: str) -> None: + resp = requests.delete(f"{self.base_url}/scopes/{scope_id}", timeout=30) + if resp.status_code not in (200, 204, 404): + resp.raise_for_status() + + def refresh_all(self) -> None: + # publishes a refresh on the webhook topic; leader pulls all scopes + resp = requests.post(f"{self.base_url}/scopes/refresh", timeout=30) + if resp.status_code == 404: + return # endpoint name differs across versions; caller falls back to poll + resp.raise_for_status() + + +def gitea_repo_url(name: str) -> str: + # url reachable from inside the opal_server container + return f"{GITEA_INTERNAL_URL}/{GITEA_USER}/{name}.git" + + +def compose(*args: str) -> subprocess.CompletedProcess: + return subprocess.run( + ["docker", "compose", *args], + cwd=_COMPOSE_DIR, + capture_output=True, + text=True, + check=True, + ) + + +def bounce_postgres(down_seconds: int = 5) -> None: + compose("stop", "postgres") + time.sleep(down_seconds) + compose("start", "postgres") + + +def list_seeded_repos(count: int) -> List[str]: + return [f"policy-repo-{i:04d}" for i in range(count)] diff --git a/app-tests/git-leak/seed/Dockerfile b/app-tests/git-leak/seed/Dockerfile new file mode 100644 index 000000000..ee9d776d2 --- /dev/null +++ b/app-tests/git-leak/seed/Dockerfile @@ -0,0 +1,6 @@ +FROM python:3.11-slim +RUN apt-get update && apt-get install -y --no-install-recommends git \ + && rm -rf /var/lib/apt/lists/* +RUN pip install --no-cache-dir requests GitPython +COPY seed_gitea.py /seed_gitea.py +ENTRYPOINT ["python", "/seed_gitea.py"] diff --git a/app-tests/git-leak/seed/seed_gitea.py b/app-tests/git-leak/seed/seed_gitea.py new file mode 100644 index 000000000..fb41dbc2a --- /dev/null +++ b/app-tests/git-leak/seed/seed_gitea.py @@ -0,0 +1,130 @@ +"""Seed a Gitea instance with N policy repos for the OPAL git-leak test bed. + +Idempotent: re-running creates only the missing repos. Each repo gets a +single commit containing a minimal OPA policy tree. + +Env: + GITEA_URL e.g. http://gitea:3000 + GITEA_ADMIN_USER admin username (created out-of-band by compose) + GITEA_ADMIN_PASSWORD admin password + REPO_COUNT how many repos to ensure exist (default 50) +""" +import os +import sys +import time +from pathlib import Path + +import requests +from git import Actor, Repo + +POLICY_REGO = """package example + +default allow = false + +allow { + input.user == "admin" +} +""" + +DATA_JSON = '{"roles": {"admin": ["read", "write"]}}\n' + + +def _wait_for_gitea(base_url: str, timeout: int = 120) -> None: + deadline = time.time() + timeout + while time.time() < deadline: + try: + if requests.get(f"{base_url}/api/v1/version", timeout=5).status_code == 200: + return + except requests.RequestException: + pass + time.sleep(2) + raise RuntimeError(f"Gitea not reachable at {base_url} within {timeout}s") + + +def _ensure_token(base_url: str, user: str, password: str) -> str: + name = "seed-token" + resp = requests.post( + f"{base_url}/api/v1/users/{user}/tokens", + auth=(user, password), + json={"name": name, "scopes": ["write:repository", "write:user"]}, + timeout=10, + ) + if resp.status_code == 201: + return resp.json()["sha1"] + # token already exists -> delete then recreate (Gitea won't reveal an existing secret) + requests.delete( + f"{base_url}/api/v1/users/{user}/tokens/{name}", auth=(user, password), timeout=10 + ) + resp = requests.post( + f"{base_url}/api/v1/users/{user}/tokens", + auth=(user, password), + json={"name": name, "scopes": ["write:repository", "write:user"]}, + timeout=10, + ) + resp.raise_for_status() + return resp.json()["sha1"] + + +def _ensure_repo(base_url: str, token: str, user: str, name: str) -> None: + headers = {"Authorization": f"token {token}"} + exists = requests.get( + f"{base_url}/api/v1/repos/{user}/{name}", headers=headers, timeout=10 + ) + if exists.status_code == 200: + return + created = requests.post( + f"{base_url}/api/v1/user/repos", + headers=headers, + json={"name": name, "private": False, "auto_init": False}, + timeout=10, + ) + created.raise_for_status() + + +def _push_policy(base_url: str, token: str, user: str, name: str, workdir: Path) -> None: + repo_dir = workdir / name + repo_dir.mkdir(parents=True, exist_ok=True) + (repo_dir / "example.rego").write_text(POLICY_REGO) + (repo_dir / "data.json").write_text(DATA_JSON) + + repo = Repo.init(repo_dir, initial_branch="main") + repo.index.add(["example.rego", "data.json"]) + author = Actor("seed", "seed@example.com") + repo.index.commit("seed policy", author=author, committer=author) + + push_url = base_url.replace("http://", f"http://{user}:{token}@") + f"/{user}/{name}.git" + origin = repo.create_remote("origin", push_url) + origin.push(refspec="main:main") + + +def main() -> int: + base_url = os.environ["GITEA_URL"].rstrip("/") + user = os.environ["GITEA_ADMIN_USER"] + password = os.environ["GITEA_ADMIN_PASSWORD"] + count = int(os.environ.get("REPO_COUNT", "50")) + + _wait_for_gitea(base_url) + token = _ensure_token(base_url, user, password) + + workdir = Path("/tmp/seed-work") + for i in range(count): + name = f"policy-repo-{i:04d}" + _ensure_repo(base_url, token, user, name) + # only push if the repo is empty (freshly created) + head = requests.get( + f"{base_url}/api/v1/repos/{user}/{name}/branches/main", + headers={"Authorization": f"token {token}"}, + timeout=10, + ) + if head.status_code != 200: + _push_policy(base_url, token, user, name, workdir) + print(f"seeded {name}", flush=True) + + # write the token where the test harness can read it + Path("/seed-output/token").write_text(token) + print(f"DONE: ensured {count} repos", flush=True) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/app-tests/git-leak/test_boot.py b/app-tests/git-leak/test_boot.py new file mode 100644 index 000000000..b9a80b5d7 --- /dev/null +++ b/app-tests/git-leak/test_boot.py @@ -0,0 +1,37 @@ +import os +import time + +import pytest + +from helpers import compose, gitea_repo_url, list_seeded_repos + + +@pytest.mark.timeout(2400) +def test_boot_loads_all_scopes(opal, repo_count): + """Measure how long a fresh boot takes to load all scope repos. + + On master this is serial and slow (the ~20-min problem at scale). + PR4 tightens BOOT_TARGET_SECONDS to assert the parallel speedup. + """ + n = repo_count + repos = list_seeded_repos(n) + for i, name in enumerate(repos): + opal.put_scope(f"boot-{i}", gitea_repo_url(name)) + + # restart only the server so it re-runs sync_scopes on boot + compose("restart", "opal_server") + opal.wait_healthy(timeout=600) + + start = time.time() + deadline = start + 2000 + while time.time() < deadline: + if opal.stats()["repos"] >= n: + break + time.sleep(2) + elapsed = time.time() - start + + # PR1 records the baseline (loose). PR4 will set BOOT_TARGET_SECONDS low. + BOOT_TARGET_SECONDS = int(os.environ.get("BOOT_TARGET_SECONDS", "2000")) + print(f"boot loaded {n} scopes in {elapsed:.1f}s (target {BOOT_TARGET_SECONDS}s)") + assert opal.stats()["repos"] >= n, "not all scopes loaded after boot" + assert elapsed < BOOT_TARGET_SECONDS, f"boot too slow: {elapsed:.1f}s" diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py new file mode 100644 index 000000000..76db3729c --- /dev/null +++ b/app-tests/git-leak/test_leak.py @@ -0,0 +1,56 @@ +import time + +import pytest + +from helpers import gitea_repo_url, list_seeded_repos + + +def _wait_until(predicate, timeout=30, interval=0.5): + deadline = time.time() + timeout + while time.time() < deadline: + if predicate(): + return True + time.sleep(interval) + return False + + +@pytest.mark.timeout(900) +def test_churn_releases_caches(opal, repo_count): + """Create then delete many scopes; the three caches must return to empty. + + FAILS on master: delete_scope never purges GitPolicyFetcher caches. + """ + n = min(repo_count, 100) + repos = list_seeded_repos(n) + for i, name in enumerate(repos): + opal.put_scope(f"churn-{i}", gitea_repo_url(name)) + _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) + + for i in range(n): + opal.delete_scope(f"churn-{i}") + + released = _wait_until(lambda: opal.stats()["repos"] == 0, timeout=30) + stats = opal.stats() + assert released, f"repos cache did not drain: {stats}" + assert stats["repos_last_fetched"] == 0, stats + + +@pytest.mark.timeout(900) +def test_repeat_sync_does_not_grow(opal, repo_count): + """Re-syncing the same scopes must not grow the caches unboundedly. + + FAILS on master: repos cache only ever grows. + """ + n = min(repo_count, 50) + repos = list_seeded_repos(n) + for i, name in enumerate(repos): + opal.put_scope(f"stable-{i}", gitea_repo_url(name)) + _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) + + baseline = opal.stats()["repos"] + for _ in range(10): + opal.refresh_all() + time.sleep(2) + + grown = opal.stats()["repos"] + assert grown <= baseline, f"repos cache grew on repeat sync: {baseline} -> {grown}" diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py new file mode 100644 index 000000000..64598d741 --- /dev/null +++ b/app-tests/git-leak/test_resilience.py @@ -0,0 +1,54 @@ +import time + +import pytest + +from helpers import bounce_postgres, gitea_repo_url, list_seeded_repos + + +@pytest.mark.timeout(300) +def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): + """An unreachable repo must not stop healthy scopes from serving. + + FAILS on master: the scopes path has no fetch timeout, so a hung + clone occupies the shared executor and the server stalls. + """ + # a routable-but-dead address: TEST-NET-1 (RFC 5737), no git server there + opal.put_scope("offline", "http://192.0.2.1/dead/repo.git", branch="main") + + healthy = list_seeded_repos(1)[0] + opal.put_scope("healthy", gitea_repo_url(healthy)) + + # the healthy scope's repo must appear in the cache within a bounded time, + # even though the offline scope is hanging + deadline = time.time() + 60 + served = False + while time.time() < deadline: + if opal.stats()["repos"] >= 1: + served = True + break + time.sleep(2) + assert served, "healthy scope never loaded while an offline repo was hanging" + + +@pytest.mark.timeout(300) +def test_server_recovers_after_postgres_bounce(opal): + """A transient Postgres outage must not leave the server permanently down. + + FAILS on master: broadcaster disconnect SIGTERMs the worker with no + in-process reconnect; after a bounce the /internal stats endpoint does + not come back within the window without an external supervisor restart. + """ + assert opal.stats() # healthy before + bounce_postgres(down_seconds=5) + + deadline = time.time() + 60 + recovered = False + while time.time() < deadline: + try: + opal.wait_healthy(timeout=5) + opal.stats() + recovered = True + break + except Exception: + time.sleep(2) + assert recovered, "server did not recover within 60s of a postgres bounce" From bd49676013111ac92fe2612d81e642d8d6509d9c Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 12:37:24 +0300 Subject: [PATCH 03/17] test(git-leak): add GiteaAdmin and make_repo_unreachable helpers Complete the helpers.py surface promised in the plan's file-structure table. Both are now functional and used, not dead code: - make_repo_unreachable(name): returns a git URL on a routable-but-dead TEST-NET-1 host (RFC 5737). test_offline_repo now uses it instead of an inlined literal. - GiteaAdmin: host-side Gitea admin client (list_repos / repo_exists / create_repo / delete_repo), exposed as the `gitea_admin` pytest fixture for tests that need to inspect or stage repos beyond the seed sidecar. Gitea is published on host port 13000 (uncommon, to avoid the usual :3000 clash) so GiteaAdmin can reach it; opal_server and the seed sidecar still use the internal http://gitea:3000. README updated with the helper and port notes. Verified live: GiteaAdmin lists the seeded repos and round-trips create/exists/delete against Gitea over the published port. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/README.md | 10 +++ app-tests/git-leak/conftest.py | 8 ++- app-tests/git-leak/docker-compose.yml | 7 ++- app-tests/git-leak/helpers.py | 88 +++++++++++++++++++++++++++ app-tests/git-leak/test_resilience.py | 11 +++- 5 files changed, 118 insertions(+), 6 deletions(-) diff --git a/app-tests/git-leak/README.md b/app-tests/git-leak/README.md index 33a291f86..bf4060b33 100644 --- a/app-tests/git-leak/README.md +++ b/app-tests/git-leak/README.md @@ -7,6 +7,16 @@ memory leak, offline-repo hang, slow serial boot, broadcaster no-reconnect. - `opal_server` (2 workers, scopes on, Postgres broadcaster, built from `docker/Dockerfile`) - `redis`, `postgres`, `gitea` (+ one-shot `gitea-admin` and `seed` sidecars) +Only `opal_server` (`:7002`) and `gitea` (`:13000` on the host) are published; +Postgres is internal to the compose network. + +## Helpers (`helpers.py`) +- `OpalServerClient` — drive opal over HTTP (`stats`, `put_scope`, `delete_scope`, `refresh_all`). +- `GiteaAdmin` — host-side Gitea admin client (`list_repos`, `repo_exists`, + `create_repo`, `delete_repo`); also exposed as the `gitea_admin` pytest fixture. +- `make_repo_unreachable(name)` — git URL on a routable-but-dead host (TEST-NET-1) for the offline-repo test. +- `bounce_postgres(down_seconds)` — stop/start Postgres to simulate a broadcaster outage. + ## Run ```bash cd app-tests/git-leak diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index 8c538aa06..ba7052079 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -2,7 +2,7 @@ import pytest -from helpers import OpalServerClient, compose +from helpers import GiteaAdmin, OpalServerClient, compose def pytest_addoption(parser): @@ -42,3 +42,9 @@ def stack(request, repo_count): @pytest.fixture() def opal(stack) -> OpalServerClient: return stack + + +@pytest.fixture(scope="session") +def gitea_admin(stack) -> GiteaAdmin: + """Host-side Gitea admin client (depends on `stack` so Gitea is up).""" + return GiteaAdmin() diff --git a/app-tests/git-leak/docker-compose.yml b/app-tests/git-leak/docker-compose.yml index 8f9f474f9..3704e5c83 100644 --- a/app-tests/git-leak/docker-compose.yml +++ b/app-tests/git-leak/docker-compose.yml @@ -30,8 +30,11 @@ services: GITEA__security__INSTALL_LOCK: "true" GITEA__server__ROOT_URL: "http://gitea:3000/" GITEA__database__DB_TYPE: "sqlite3" - # not published to the host: only the seed sidecar and opal_server reach it - # over the compose network (via http://gitea:3000). Avoids host port clashes. + # published on 13000 (not 3000) for the host-side GiteaAdmin helper; the + # uncommon port avoids the usual :3000 clash. opal_server and the seed + # sidecar still reach it over the compose network via http://gitea:3000. + ports: + - "13000:3000" volumes: - gitea-data:/data healthcheck: diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 93871f194..936a7fc61 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -7,8 +7,16 @@ import requests OPAL_URL = "http://localhost:7002" +# reachable from inside the opal_server container (compose network) GITEA_INTERNAL_URL = "http://gitea:3000" +# reachable from the host-side test harness (published port, see docker-compose.yml) +GITEA_HOST_URL = "http://localhost:13000" GITEA_USER = "opaladmin" +GITEA_PASSWORD = "opaladmin" + +# TEST-NET-1 (RFC 5737): routable but runs no git server, so a clone hangs +# instead of failing fast — used to simulate an offline/unreachable repo. +UNREACHABLE_HOST = "192.0.2.1" # the compose project lives next to this file; compose() runs from here _COMPOSE_DIR = str(Path(__file__).resolve().parent) @@ -69,11 +77,91 @@ def refresh_all(self) -> None: resp.raise_for_status() +class GiteaAdmin: + """Host-side admin client for the test bed's Gitea. + + The ``seed`` sidecar does the bulk repo creation from inside the compose + network; this class lets a test inspect or mutate Gitea repos directly + from the host (e.g. assert seeding happened, or add/remove a single repo + for a specific scenario). It authenticates with the admin user that the + ``gitea-admin`` sidecar created, over the published host port. + """ + + def __init__( + self, + base_url: str = GITEA_HOST_URL, + user: str = GITEA_USER, + password: str = GITEA_PASSWORD, + ): + self.base_url = base_url.rstrip("/") + self._user = user + self._auth = (user, password) + + def repo_exists(self, name: str) -> bool: + resp = requests.get( + f"{self.base_url}/api/v1/repos/{self._user}/{name}", + auth=self._auth, + timeout=10, + ) + return resp.status_code == 200 + + def list_repos(self) -> List[str]: + names: List[str] = [] + page = 1 + while True: + resp = requests.get( + f"{self.base_url}/api/v1/users/{self._user}/repos", + params={"page": page, "limit": 50}, + auth=self._auth, + timeout=10, + ) + resp.raise_for_status() + batch = resp.json() + if not batch: + break + names.extend(r["name"] for r in batch) + page += 1 + return names + + def create_repo(self, name: str) -> None: + if self.repo_exists(name): + return + resp = requests.post( + f"{self.base_url}/api/v1/user/repos", + json={"name": name, "private": False, "auto_init": True}, + auth=self._auth, + timeout=10, + ) + resp.raise_for_status() + + def delete_repo(self, name: str) -> None: + resp = requests.delete( + f"{self.base_url}/api/v1/repos/{self._user}/{name}", + auth=self._auth, + timeout=10, + ) + if resp.status_code not in (204, 404): + resp.raise_for_status() + + def gitea_repo_url(name: str) -> str: # url reachable from inside the opal_server container return f"{GITEA_INTERNAL_URL}/{GITEA_USER}/{name}.git" +def make_repo_unreachable(name: str) -> str: + """Return a git URL for ``name`` pointing at a routable-but-dead host. + + Simulates an offline/unreachable policy repo: the address is in + TEST-NET-1 (RFC 5737), which is routable but runs no git server, so a + clone hangs rather than failing fast — exercising the missing fetch + timeout on the scopes path (the bug PR3 fixes). The URL keeps the same + ``/{user}/{name}.git`` shape as a real Gitea repo so the scope looks + ordinary apart from the unreachable host. + """ + return f"http://{UNREACHABLE_HOST}/{GITEA_USER}/{name}.git" + + def compose(*args: str) -> subprocess.CompletedProcess: return subprocess.run( ["docker", "compose", *args], diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index 64598d741..7d9427903 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -2,7 +2,12 @@ import pytest -from helpers import bounce_postgres, gitea_repo_url, list_seeded_repos +from helpers import ( + bounce_postgres, + gitea_repo_url, + list_seeded_repos, + make_repo_unreachable, +) @pytest.mark.timeout(300) @@ -12,8 +17,8 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): FAILS on master: the scopes path has no fetch timeout, so a hung clone occupies the shared executor and the server stalls. """ - # a routable-but-dead address: TEST-NET-1 (RFC 5737), no git server there - opal.put_scope("offline", "http://192.0.2.1/dead/repo.git", branch="main") + # a routable-but-dead address (TEST-NET-1, RFC 5737): the clone hangs + opal.put_scope("offline", make_repo_unreachable("dead-repo"), branch="main") healthy = list_seeded_repos(1)[0] opal.put_scope("healthy", gitea_repo_url(healthy)) From afeb9698ae65c3d4e373cc8468b75ae87149f68b Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 12:55:54 +0300 Subject: [PATCH 04/17] test(git-leak): correct postgres-bounce framing (passes on master) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified test_server_recovers_after_postgres_bounce against the stack: it PASSES on master (~14-19s). On a broadcaster drop the affected worker triggers a graceful shutdown, gunicorn respawns it, and the sibling worker keeps serving HTTP, so the surface recovers within the window — recovery happens via gunicorn's in-container worker supervision, not an external supervisor and not an in-process reconnect. Reframe #5 as a recovery guard (not a known-broken case) in the docstring and README; the prior "FAILS on master / needs external supervisor" wording was wrong. PER-15065's in-process reconnect would avoid the worker churn but recovery already holds. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/README.md | 9 +++++++-- app-tests/git-leak/test_resilience.py | 13 ++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app-tests/git-leak/README.md b/app-tests/git-leak/README.md index bf4060b33..778971705 100644 --- a/app-tests/git-leak/README.md +++ b/app-tests/git-leak/README.md @@ -27,8 +27,13 @@ Useful flags: `--boot-scopes=N` (any N), `--keep-stack` (skip teardown), env `BOOT_TARGET_SECONDS=120` (tighten the boot gate). ## Expected on master -All five flagship tests FAIL (except the boot test, which only fails when -`BOOT_TARGET_SECONDS` is set low). They become the regression gates for the fixes. +The leak tests (#1, #2) and the offline-repo test (#4) FAIL on master — they +target unfixed bugs and become the regression gates for PR2/PR3. The boot test +(#3) passes but only fails when `BOOT_TARGET_SECONDS` is set low (PR4's gate). +The Postgres-bounce test (#5) PASSES on master: it is a recovery guard — when +the broadcaster drops, the worker is respawned by gunicorn while the sibling +worker keeps serving, so the HTTP surface recovers. It guards that property +against regression rather than reproducing a current failure. ## Requires Docker + docker compose v2, plus host Python with `pytest pytest-timeout requests GitPython`. diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index 7d9427903..8455451eb 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -37,11 +37,14 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): @pytest.mark.timeout(300) def test_server_recovers_after_postgres_bounce(opal): - """A transient Postgres outage must not leave the server permanently down. - - FAILS on master: broadcaster disconnect SIGTERMs the worker with no - in-process reconnect; after a bounce the /internal stats endpoint does - not come back within the window without an external supervisor restart. + """A transient Postgres (broadcaster) outage must not leave the server down. + + Recovery guard, not a known-broken case. On current master this PASSES: + when the broadcast channel drops, the affected worker triggers a graceful + shutdown and gunicorn respawns it, while the sibling worker keeps serving + HTTP — so the surface recovers within the window. It guards against a + regression of that property (PER-15065's in-process reconnect would make + recovery cleaner by avoiding the worker churn, but recovery already holds). """ assert opal.stats() # healthy before bounce_postgres(down_seconds=5) From 92353f6277521da05e1f0e5eb782314e4e424fb0 Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 13:03:05 +0300 Subject: [PATCH 05/17] style(git-leak): apply black/isort/docformatter (pre-commit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run the repo's pinned pre-commit formatters (black 23.1.0, isort 5.12.0, docformatter 1.7.5) over the PR1 files to satisfy the pre-commit CI check. Formatting only — no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/conftest.py | 1 - app-tests/git-leak/helpers.py | 5 ++++- app-tests/git-leak/seed/seed_gitea.py | 12 +++++++++--- app-tests/git-leak/test_boot.py | 1 - app-tests/git-leak/test_leak.py | 1 - app-tests/git-leak/test_resilience.py | 19 ++++++++++--------- packages/opal-server/opal_server/server.py | 2 +- .../tests/debug_stats_endpoint_test.py | 1 - 8 files changed, 24 insertions(+), 18 deletions(-) diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index ba7052079..7501f99d0 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -1,7 +1,6 @@ import os import pytest - from helpers import GiteaAdmin, OpalServerClient, compose diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 936a7fc61..1a3bd4fcc 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -31,7 +31,10 @@ def wait_healthy(self, timeout: int = 180) -> None: last = None while time.time() < deadline: try: - if requests.get(f"{self.base_url}/healthcheck", timeout=5).status_code == 200: + if ( + requests.get(f"{self.base_url}/healthcheck", timeout=5).status_code + == 200 + ): return except requests.RequestException as exc: last = exc diff --git a/app-tests/git-leak/seed/seed_gitea.py b/app-tests/git-leak/seed/seed_gitea.py index fb41dbc2a..ddebe5667 100644 --- a/app-tests/git-leak/seed/seed_gitea.py +++ b/app-tests/git-leak/seed/seed_gitea.py @@ -53,7 +53,9 @@ def _ensure_token(base_url: str, user: str, password: str) -> str: return resp.json()["sha1"] # token already exists -> delete then recreate (Gitea won't reveal an existing secret) requests.delete( - f"{base_url}/api/v1/users/{user}/tokens/{name}", auth=(user, password), timeout=10 + f"{base_url}/api/v1/users/{user}/tokens/{name}", + auth=(user, password), + timeout=10, ) resp = requests.post( f"{base_url}/api/v1/users/{user}/tokens", @@ -81,7 +83,9 @@ def _ensure_repo(base_url: str, token: str, user: str, name: str) -> None: created.raise_for_status() -def _push_policy(base_url: str, token: str, user: str, name: str, workdir: Path) -> None: +def _push_policy( + base_url: str, token: str, user: str, name: str, workdir: Path +) -> None: repo_dir = workdir / name repo_dir.mkdir(parents=True, exist_ok=True) (repo_dir / "example.rego").write_text(POLICY_REGO) @@ -92,7 +96,9 @@ def _push_policy(base_url: str, token: str, user: str, name: str, workdir: Path) author = Actor("seed", "seed@example.com") repo.index.commit("seed policy", author=author, committer=author) - push_url = base_url.replace("http://", f"http://{user}:{token}@") + f"/{user}/{name}.git" + push_url = ( + base_url.replace("http://", f"http://{user}:{token}@") + f"/{user}/{name}.git" + ) origin = repo.create_remote("origin", push_url) origin.push(refspec="main:main") diff --git a/app-tests/git-leak/test_boot.py b/app-tests/git-leak/test_boot.py index b9a80b5d7..928fd6f4e 100644 --- a/app-tests/git-leak/test_boot.py +++ b/app-tests/git-leak/test_boot.py @@ -2,7 +2,6 @@ import time import pytest - from helpers import compose, gitea_repo_url, list_seeded_repos diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 76db3729c..30b9a81a0 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -1,7 +1,6 @@ import time import pytest - from helpers import gitea_repo_url, list_seeded_repos diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index 8455451eb..b8fb73c89 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -1,7 +1,6 @@ import time import pytest - from helpers import ( bounce_postgres, gitea_repo_url, @@ -37,14 +36,16 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): @pytest.mark.timeout(300) def test_server_recovers_after_postgres_bounce(opal): - """A transient Postgres (broadcaster) outage must not leave the server down. - - Recovery guard, not a known-broken case. On current master this PASSES: - when the broadcast channel drops, the affected worker triggers a graceful - shutdown and gunicorn respawns it, while the sibling worker keeps serving - HTTP — so the surface recovers within the window. It guards against a - regression of that property (PER-15065's in-process reconnect would make - recovery cleaner by avoiding the worker churn, but recovery already holds). + """A transient Postgres (broadcaster) outage must not leave the server + down. + + Recovery guard, not a known-broken case. On current master this + PASSES: when the broadcast channel drops, the affected worker + triggers a graceful shutdown and gunicorn respawns it, while the + sibling worker keeps serving HTTP — so the surface recovers within + the window. It guards against a regression of that property + (PER-15065's in-process reconnect would make recovery cleaner by + avoiding the worker churn, but recovery already holds). """ assert opal.stats() # healthy before bounce_postgres(down_seconds=5) diff --git a/packages/opal-server/opal_server/server.py b/packages/opal-server/opal_server/server.py index 2b88b3db8..1be796d6d 100644 --- a/packages/opal-server/opal_server/server.py +++ b/packages/opal-server/opal_server/server.py @@ -26,8 +26,8 @@ ) from opal_server.config import opal_server_config from opal_server.data.api import init_data_updates_router -from opal_server.debug_stats import register_internal_stats_route from opal_server.data.data_update_publisher import DataUpdatePublisher +from opal_server.debug_stats import register_internal_stats_route from opal_server.loadlimiting import init_loadlimit_router from opal_server.policy.bundles.api import router as bundles_router from opal_server.policy.watcher.factory import setup_watcher_task diff --git a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py index 8ba0f678a..00a9c8e1b 100644 --- a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py +++ b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py @@ -1,6 +1,5 @@ from fastapi import FastAPI from fastapi.testclient import TestClient - from opal_server.debug_stats import register_internal_stats_route From db54ae627824aaea11f72b3ff829dcaddc6fa96d Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 13:13:22 +0300 Subject: [PATCH 06/17] test: scope root pytest collection to packages/ (exclude git-leak bed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CI `build` job runs `pytest` from the repo root with no path, which recursed into app-tests/git-leak/ and ran the flagship tests — these are designed to FAIL on master (they are the regression gates for PR2-PR5), so they broke the build job. Set `testpaths = packages` so the rootdir run collects only the unit tests under packages/ (matching master's effective behavior, since app-tests/ had no pytest files before). testpaths only applies when pytest is invoked from the rootdir with no args, so `cd app-tests/git-leak && pytest` still collects and runs the test bed. Verified both: root run -> packages only; subdir run -> all 5 flagship tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- pytest.ini | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pytest.ini b/pytest.ini index 16c88ba91..c98ec88b9 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,3 +1,10 @@ # Handling DeprecationWarning 'asyncio_mode' default value [pytest] asyncio_mode = strict +# Scope the default (rootdir) collection to the unit tests under packages/. +# The heavyweight, docker-compose-driven test bed under app-tests/git-leak/ is +# designed to FAIL on master (it is the regression gate for later PRs) and must +# not be collected by the repo's CI `pytest` run. testpaths only applies when +# pytest is invoked from the rootdir with no args, so running it explicitly +# (e.g. `cd app-tests/git-leak && pytest`) still works. +testpaths = packages From 6f9a0894a1aa937a91a0cbe8f12b9b666306b680 Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 13:24:22 +0300 Subject: [PATCH 07/17] test(git-leak): address Copilot review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - stats(): sample the /internal endpoint several times and merge per key with max(). The caches are per-process on a multi-worker server, so a single read can hit an empty (non-leader) worker — max-merge avoids both false negatives (missing a populated leader) and false positives (an `== 0` drain assertion passing only because it hit an empty worker). - test_leak: assert the initial-load `_wait_until` succeeded before deleting / before taking baseline, so the tests can't pass vacuously when load never completed. - refresh_all(): correct the misleading comment — a 404 is a no-op, there is no client-side fallback. - conftest: skip the suite cleanly if docker is unavailable (defense in depth; it's already excluded from the default pytest run via testpaths). Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/conftest.py | 7 +++++++ app-tests/git-leak/helpers.py | 35 +++++++++++++++++++++++++-------- app-tests/git-leak/test_leak.py | 6 ++++-- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index 7501f99d0..9701aff87 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -1,4 +1,5 @@ import os +import shutil import pytest from helpers import GiteaAdmin, OpalServerClient, compose @@ -26,6 +27,12 @@ def repo_count(request) -> int: @pytest.fixture(scope="session") def stack(request, repo_count): + # Defense-in-depth: this docker-compose suite is already excluded from the + # repo's default `pytest` run via `testpaths = packages` in pytest.ini, so + # the unit-test CI matrix never collects it. If it is ever collected in an + # environment without docker, skip cleanly instead of erroring. + if shutil.which("docker") is None: + pytest.skip("docker (compose) is required for the git-leak test bed") os.environ["REPO_COUNT"] = str(repo_count) # build + start infra; seed runs to completion then exits compose("up", "-d", "--build") diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 1a3bd4fcc..8d0acc38a 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -41,12 +41,28 @@ def wait_healthy(self, timeout: int = 180) -> None: time.sleep(2) raise RuntimeError(f"opal-server not healthy in {timeout}s (last: {last})") - def stats(self) -> Dict[str, int]: - resp = requests.get( - f"{self.base_url}/internal/git-fetcher-cache-stats", timeout=10 - ) - resp.raise_for_status() - return resp.json() + def stats(self, samples: int = 5, interval: float = 0.1) -> Dict[str, int]: + """Read the git-fetcher cache stats, merged across several reads. + + The server runs multiple workers and the ``GitPolicyFetcher`` caches + are per-process, so a single read may land on a worker with empty + caches (e.g. a non-leader that never fetched). Sample a few times and + take the ``max`` per key, so the harness observes whichever worker + still holds the most entries — this prevents both false negatives + (missing a populated leader) and false positives (an ``== 0`` drain + assertion passing only because it hit an empty worker). + """ + merged: Dict[str, int] = {} + for i in range(max(1, samples)): + resp = requests.get( + f"{self.base_url}/internal/git-fetcher-cache-stats", timeout=10 + ) + resp.raise_for_status() + for key, value in resp.json().items(): + merged[key] = max(merged.get(key, 0), value) + if i < samples - 1: + time.sleep(interval) + return merged def put_scope(self, scope_id: str, repo_url: str, branch: str = "main") -> None: body = { @@ -73,10 +89,13 @@ def delete_scope(self, scope_id: str) -> None: resp.raise_for_status() def refresh_all(self) -> None: - # publishes a refresh on the webhook topic; leader pulls all scopes + # Best-effort: POST /scopes/refresh publishes on the webhook topic so + # every leader re-syncs all scopes. If this OPAL build doesn't expose + # the route (404), treat it as a no-op — there is no client-side + # fallback; callers rely on their own stats polling either way. resp = requests.post(f"{self.base_url}/scopes/refresh", timeout=30) if resp.status_code == 404: - return # endpoint name differs across versions; caller falls back to poll + return resp.raise_for_status() diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 30b9a81a0..d2ef0f21b 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -23,7 +23,8 @@ def test_churn_releases_caches(opal, repo_count): repos = list_seeded_repos(n) for i, name in enumerate(repos): opal.put_scope(f"churn-{i}", gitea_repo_url(name)) - _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) + loaded = _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) + assert loaded, f"initial load never reached {n} repos: {opal.stats()}" for i in range(n): opal.delete_scope(f"churn-{i}") @@ -44,7 +45,8 @@ def test_repeat_sync_does_not_grow(opal, repo_count): repos = list_seeded_repos(n) for i, name in enumerate(repos): opal.put_scope(f"stable-{i}", gitea_repo_url(name)) - _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) + loaded = _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) + assert loaded, f"initial sync never reached {n} repos: {opal.stats()}" baseline = opal.stats()["repos"] for _ in range(10): From 1b23ac00223f3b827d51546e0be6c196c27a8edf Mon Sep 17 00:00:00 2001 From: David Shoen Date: Tue, 23 Jun 2026 13:47:25 +0300 Subject: [PATCH 08/17] test(git-leak): isolate scopes per test and fix false repeat-sync gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the regression gates: - Cross-test contamination: clone paths are keyed by repo URL (source_id = sha256(url)+branch-shard), not scope_id, so scopes from different tests that point at the same seeded repo share one GitPolicyFetcher cache entry. With a session-scoped stack and no teardown, a leftover boot-*/stable-* scope kept those entries alive and would make test_churn's `repos == 0` drain assertion fail on fixed code. OpalServerClient now tracks created scopes and the opal fixture deletes them on teardown (best-effort drain wait, swallows errors so master — where delete never purges — doesn't fail the passing test). - False gate: test_repeat_sync_does_not_grow re-syncs identical scopes, which a path-keyed cache can't grow even on master, so it could never be the leak gate it claimed. Reframed as an honest idempotency guard (passes on master) that points at test_churn_releases_caches as the real leak gate; README's "Expected on master" reclassifies it alongside the postgres-bounce guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/README.md | 21 ++++++++++++++------- app-tests/git-leak/conftest.py | 9 ++++++++- app-tests/git-leak/helpers.py | 31 +++++++++++++++++++++++++++++++ app-tests/git-leak/test_leak.py | 11 +++++++++-- 4 files changed, 62 insertions(+), 10 deletions(-) diff --git a/app-tests/git-leak/README.md b/app-tests/git-leak/README.md index 778971705..a8abea9f6 100644 --- a/app-tests/git-leak/README.md +++ b/app-tests/git-leak/README.md @@ -27,13 +27,20 @@ Useful flags: `--boot-scopes=N` (any N), `--keep-stack` (skip teardown), env `BOOT_TARGET_SECONDS=120` (tighten the boot gate). ## Expected on master -The leak tests (#1, #2) and the offline-repo test (#4) FAIL on master — they -target unfixed bugs and become the regression gates for PR2/PR3. The boot test -(#3) passes but only fails when `BOOT_TARGET_SECONDS` is set low (PR4's gate). -The Postgres-bounce test (#5) PASSES on master: it is a recovery guard — when -the broadcaster drops, the worker is respawned by gunicorn while the sibling -worker keeps serving, so the HTTP surface recovers. It guards that property -against regression rather than reproducing a current failure. +The churn leak test (`test_churn_releases_caches`) and the offline-repo test +FAIL on master — they target unfixed bugs and become the regression gates for +PR2/PR3. The boot test passes but only fails when `BOOT_TARGET_SECONDS` is set +low (PR4's gate). + +Two tests are guards that PASS on master rather than reproducing a current +failure: +- `test_repeat_sync_does_not_grow` — clone paths are keyed by the repo URL, so + re-syncing identical scopes reuses cache entries and does not grow them. It + guards against a regression that would make repeat sync allocate per-sync. + (The unbounded-growth-then-no-purge leak is the *churn* test's job.) +- `test_server_recovers_after_postgres_bounce` — when the broadcaster drops, the + affected worker is respawned by gunicorn while the sibling keeps serving, so + the HTTP surface recovers. Guards that property against regression. ## Requires Docker + docker compose v2, plus host Python with `pytest pytest-timeout requests GitPython`. diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index 9701aff87..c3c01bdbb 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -47,7 +47,14 @@ def stack(request, repo_count): @pytest.fixture() def opal(stack) -> OpalServerClient: - return stack + # The compose stack is session-scoped (one server for the whole run), but + # scopes must not leak between tests: clone paths are keyed by repo URL, so + # a scope left behind by one test shares a cache entry with any later test + # using the same seeded repo and would pollute its drain assertions. Delete + # whatever this test created on teardown to keep each test isolated. + stack._created_scopes.clear() + yield stack + stack.cleanup_created_scopes() @pytest.fixture(scope="session") diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 8d0acc38a..4fefaf81d 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -25,6 +25,12 @@ class OpalServerClient: def __init__(self, base_url: str = OPAL_URL): self.base_url = base_url.rstrip("/") + # scope_ids created via put_scope, so a per-test fixture can delete them + # on teardown. Clone paths are keyed by repo URL (not scope_id), so a + # scope left behind by one test shares a GitPolicyFetcher cache entry + # with any other test pointing at the same seeded repo — without cleanup + # that leftover keeps the entry alive and pollutes a drain assertion. + self._created_scopes: set = set() def wait_healthy(self, timeout: int = 180) -> None: deadline = time.time() + timeout @@ -82,11 +88,36 @@ def put_scope(self, scope_id: str, repo_url: str, branch: str = "main") -> None: # the scope router mounts at prefix="/scopes" with @router.put("") resp = requests.put(f"{self.base_url}/scopes", json=body, timeout=30) resp.raise_for_status() + self._created_scopes.add(scope_id) def delete_scope(self, scope_id: str) -> None: resp = requests.delete(f"{self.base_url}/scopes/{scope_id}", timeout=30) if resp.status_code not in (200, 204, 404): resp.raise_for_status() + self._created_scopes.discard(scope_id) + + def cleanup_created_scopes(self, drain_timeout: int = 15) -> None: + """Delete every scope created on this client, then best-effort wait for + the caches to drain — so the next test starts from a clean slate. + + Best-effort by design: on master, delete never purges the caches (the + leak this suite gates), so the drain wait will simply time out. A + teardown failure must never fail the test that just ran, hence the broad + excepts and the bounded wait. + """ + for scope_id in list(self._created_scopes): + try: + self.delete_scope(scope_id) + except Exception: + self._created_scopes.discard(scope_id) + deadline = time.time() + drain_timeout + while time.time() < deadline: + try: + if self.stats(samples=1)["repos"] == 0: + return + except Exception: + return + time.sleep(1) def refresh_all(self) -> None: # Best-effort: POST /scopes/refresh publishes on the webhook topic so diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index d2ef0f21b..8cfee6363 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -37,9 +37,16 @@ def test_churn_releases_caches(opal, repo_count): @pytest.mark.timeout(900) def test_repeat_sync_does_not_grow(opal, repo_count): - """Re-syncing the same scopes must not grow the caches unboundedly. + """Re-syncing the *same* scopes must not grow the caches. - FAILS on master: repos cache only ever grows. + Idempotency guard, not a known-broken case. On current master this + PASSES: a clone path is keyed by the repo URL (``source_id`` = + sha256(url)+branch-shard), so re-syncing identical scopes reuses the + existing ``repos`` / ``repos_last_fetched`` entries instead of + allocating new ones. It guards against a regression that would make + repeat sync allocate per-sync. The unbounded *growth + no purge on + delete* leak is covered by ``test_churn_releases_caches`` above, which + uses distinct scopes and DOES fail on master. """ n = min(repo_count, 50) repos = list_seeded_repos(n) From 6046a103f229c07616a9e053fb5de0a0267e93f7 Mon Sep 17 00:00:00 2001 From: David Shoen Date: Wed, 24 Jun 2026 16:03:00 +0300 Subject: [PATCH 09/17] test(git-leak): make the regression gates trustworthy (address PR review) Addresses the CHANGES_REQUESTED review on PR #922. Root cause behind most findings: a fresh scope's first sync takes the _clone() branch, which only fills GitPolicyFetcher.repo_locks; repos/repos_last_fetched are filled on a *second* sync. So the load gates on `repos` hung, and the 2-worker per-process caches made `== 0` drain assertions unsafe. Blocking: - Single worker (UVICORN_NUM_WORKERS=1): deterministic per-process cache reads; removes the false `== 0` drain class. - Load gate (CRITICAL + Zivxx HIGH): _load_scopes gates on repo_locks then refresh_all() to force the second sync, so repos/repos_last_fetched are actually populated before any drain/purge assertion. - compose() surfaces captured stdout/stderr on failure. - Seed completeness asserted in conftest; seed script isolates per-repo failures and exits non-zero with a count. Secondary: - test_repeat_sync asserts an RSS bound (count can't grow for any impl); churn asserts all three caches drain + a loose RSS backstop. - blackhole socat sidecar replaces TEST-NET-1 (deterministic hang); offline test saturates the fetch executor with 40 hung clones and recovers via OpalServerClient.hard_reset() (stop -> redis FLUSHALL -> start). - Per-test clean slate deletes all server scopes (fixes orphan-scope leak). - Postgres-bounce proves broadcast recovery (PUT post-bounce, assert sync) and uses `up -d --wait`. - Remove dead 404 branch in refresh_all; boot clock starts at restart; gitea-admin `|| true` -> "already exists"-only guard; README reworded. opal-server: - /internal stats route now takes the JWTAuthenticator dependency (protected when JWT on, no-op in the test bed); unit test asserts enforcement. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/README.md | 62 +++++--- app-tests/git-leak/conftest.py | 27 +++- app-tests/git-leak/docker-compose.yml | 26 +++- app-tests/git-leak/helpers.py | 141 ++++++++++++------ app-tests/git-leak/seed/seed_gitea.py | 36 +++-- app-tests/git-leak/test_boot.py | 12 +- app-tests/git-leak/test_leak.py | 99 ++++++++---- app-tests/git-leak/test_resilience.py | 105 +++++++++---- .../opal-server/opal_server/debug_stats.py | 25 +++- packages/opal-server/opal_server/server.py | 4 +- .../tests/debug_stats_endpoint_test.py | 19 ++- 11 files changed, 407 insertions(+), 149 deletions(-) diff --git a/app-tests/git-leak/README.md b/app-tests/git-leak/README.md index a8abea9f6..aabbb4657 100644 --- a/app-tests/git-leak/README.md +++ b/app-tests/git-leak/README.md @@ -1,21 +1,31 @@ # OPAL git-leak / resilience test bed -Reproduces (as failing tests on `master`) the four issues fixed by PR2–PR5: -memory leak, offline-repo hang, slow serial boot, broadcaster no-reconnect. +Reproduces (as failing tests) the four issues fixed by PR2–PR5: memory leak, +offline-repo hang, slow serial boot, broadcaster no-reconnect. + +Every assertion is driven through `GET /internal/git-fetcher-cache-stats`, which +**this PR (PR1) adds** — it does not exist on `master`. So the suite runs against +*this branch*: the leak/offline tests fail here *until PR2/PR3 land*, then go +green. Run against true `master` they would all error at setup on the missing +endpoint, not "fail for the targeted bug." ## Stack -- `opal_server` (2 workers, scopes on, Postgres broadcaster, built from `docker/Dockerfile`) +- `opal_server` (single worker, scopes on, Postgres broadcaster, built from `docker/Dockerfile`) - `redis`, `postgres`, `gitea` (+ one-shot `gitea-admin` and `seed` sidecars) +- `blackhole` (alpine/socat: accepts TCP then never answers — the offline repo) Only `opal_server` (`:7002`) and `gitea` (`:13000` on the host) are published; -Postgres is internal to the compose network. +Postgres and `blackhole` are internal to the compose network. ## Helpers (`helpers.py`) -- `OpalServerClient` — drive opal over HTTP (`stats`, `put_scope`, `delete_scope`, `refresh_all`). +- `OpalServerClient` — drive opal over HTTP (`stats`, `put_scope`, `delete_scope`, + `refresh_all`, `get_scope_policy`, `list_scope_ids`, `delete_all_scopes`). - `GiteaAdmin` — host-side Gitea admin client (`list_repos`, `repo_exists`, `create_repo`, `delete_repo`); also exposed as the `gitea_admin` pytest fixture. -- `make_repo_unreachable(name)` — git URL on a routable-but-dead host (TEST-NET-1) for the offline-repo test. -- `bounce_postgres(down_seconds)` — stop/start Postgres to simulate a broadcaster outage. +- `make_repo_unreachable(name)` — git URL on the `blackhole` sidecar (completes + the TCP handshake, never answers) so the clone hangs for the offline-repo test. +- `bounce_postgres(down_seconds)` — stop Postgres, then `up -d --wait` it back to + simulate a broadcaster outage and await readiness before the recovery poll. ## Run ```bash @@ -26,21 +36,21 @@ python -m pytest test_leak.py -v --boot-scopes=20 # just the leak gates Useful flags: `--boot-scopes=N` (any N), `--keep-stack` (skip teardown), env `BOOT_TARGET_SECONDS=120` (tighten the boot gate). -## Expected on master +## Expected behavior The churn leak test (`test_churn_releases_caches`) and the offline-repo test -FAIL on master — they target unfixed bugs and become the regression gates for -PR2/PR3. The boot test passes but only fails when `BOOT_TARGET_SECONDS` is set -low (PR4's gate). +FAIL on this branch *without the PR2/PR3 fix* — they target unfixed bugs and +become the regression gates for PR2/PR3, flipping green when those land. The +boot test passes but fails when `BOOT_TARGET_SECONDS` is set low (PR4's gate). -Two tests are guards that PASS on master rather than reproducing a current -failure: +Two tests are guards that PASS rather than reproducing a current failure: - `test_repeat_sync_does_not_grow` — clone paths are keyed by the repo URL, so - re-syncing identical scopes reuses cache entries and does not grow them. It - guards against a regression that would make repeat sync allocate per-sync. - (The unbounded-growth-then-no-purge leak is the *churn* test's job.) + re-syncing identical scopes reuses cache entries and the cache *counts* can't + grow for any implementation; the load-bearing assertion is therefore on RSS, + guarding against a regression that leaks per-sync allocations. - `test_server_recovers_after_postgres_bounce` — when the broadcaster drops, the - affected worker is respawned by gunicorn while the sibling keeps serving, so - the HTTP surface recovers. Guards that property against regression. + worker is respawned by gunicorn and the broadcaster reconnects once Postgres + is back; the test PUTs a fresh scope post-bounce and asserts it syncs, proving + the broadcast path (not just HTTP) recovered. ## Requires Docker + docker compose v2, plus host Python with `pytest pytest-timeout requests GitPython`. @@ -48,8 +58,14 @@ Docker + docker compose v2, plus host Python with `pytest pytest-timeout request ## Notes - Auth is disabled in the stack: `OPAL_AUTH_PUBLIC_KEY` is left unset so the JWT verifier is disabled and the harness can call scope routes without minting JWTs. - Local test bed only; never a production setting. -- The server runs 2 uvicorn workers with a Postgres broadcaster, mirroring a - realistic multi-worker deployment. The `GitPolicyFetcher` caches read by the - `/internal/git-fetcher-cache-stats` endpoint are per-process, so the harness - polls with generous timeouts to let the leader worker converge. + Local test bed only; never a production setting. (The `/internal` endpoint is + registered with the same `JWTAuthenticator` dependency as the other routes, so + it is protected when JWT verification is enabled and open only here.) +- The server runs a **single** uvicorn worker. The `GitPolicyFetcher` caches read + by `/internal/git-fetcher-cache-stats` are per-process, so a multi-worker stack + would make a round-robin read miss the worker that fetched and let a `== 0` + drain assertion pass falsely. One worker makes every cache read deterministic; + the leak/boot/offline bugs all reproduce single-worker. +- First-sync of a fresh scope takes the clone path, which fills only `repo_locks`; + `repos` / `repos_last_fetched` are filled by the discover/fetch path on a second + sync, so the load helpers issue a `refresh_all()` before asserting on `repos`. diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index c3c01bdbb..1660d596b 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -2,7 +2,7 @@ import shutil import pytest -from helpers import GiteaAdmin, OpalServerClient, compose +from helpers import GiteaAdmin, OpalServerClient, compose, list_seeded_repos def pytest_addoption(parser): @@ -36,8 +36,20 @@ def stack(request, repo_count): os.environ["REPO_COUNT"] = str(repo_count) # build + start infra; seed runs to completion then exits compose("up", "-d", "--build") - # block until seeding sidecar has finished creating repos + # block until seeding sidecar has finished creating repos. compose() raises + # (with output) if the seed container exited non-zero, so a hard seed + # failure surfaces here rather than as a confusing later test failure. compose("wait", "seed") + # Verify the seed actually produced all N repos before any test runs: a + # partial seed would otherwise look like a server bug when the load gate + # can't reach N. Fail loudly with the gap. + expected = set(list_seeded_repos(repo_count)) + present = set(GiteaAdmin().list_repos()) + missing = expected - present + assert not missing, ( + f"seed incomplete: {len(missing)}/{repo_count} repos missing " + f"(e.g. {sorted(missing)[:5]})" + ) client = OpalServerClient() client.wait_healthy() yield client @@ -50,11 +62,14 @@ def opal(stack) -> OpalServerClient: # The compose stack is session-scoped (one server for the whole run), but # scopes must not leak between tests: clone paths are keyed by repo URL, so # a scope left behind by one test shares a cache entry with any later test - # using the same seeded repo and would pollute its drain assertions. Delete - # whatever this test created on teardown to keep each test isolated. - stack._created_scopes.clear() + # using the same seeded repo and would pollute its drain assertions. + # + # Delete every scope the *server* currently knows (not just this client's + # tracked set) at setup, so a scope orphaned by a prior failed test can't + # contaminate this one; then again on teardown. + stack.delete_all_scopes() yield stack - stack.cleanup_created_scopes() + stack.delete_all_scopes() @pytest.fixture(scope="session") diff --git a/app-tests/git-leak/docker-compose.yml b/app-tests/git-leak/docker-compose.yml index 3704e5c83..bd48c26dc 100644 --- a/app-tests/git-leak/docker-compose.yml +++ b/app-tests/git-leak/docker-compose.yml @@ -51,15 +51,30 @@ services: condition: service_healthy user: git entrypoint: ["/bin/sh", "-c"] + # Tolerate only the idempotent "already exists" case; any other failure + # must abort so `seed` (which depends on this completing) doesn't run + # against a Gitea with no admin user and fail with a confusing 401. command: - > - gitea admin user create --username opaladmin --password opaladmin + out=$$(gitea admin user create --username opaladmin --password opaladmin --email admin@example.com --admin --must-change-password=false - --config /data/gitea/conf/app.ini || true + --config /data/gitea/conf/app.ini 2>&1); rc=$$?; + echo "$$out"; + if [ $$rc -ne 0 ] && ! echo "$$out" | grep -qi "already exist"; then + exit $$rc; + fi volumes: - gitea-data:/data restart: "no" + blackhole: + # Accepts the TCP handshake then never answers — a clone connects and + # blocks reading the git smart-HTTP response, holding the fetch executor. + # Deterministic, unlike a TEST-NET-1 address which many networks reject + # fast with ICMP-unreachable (so the clone would fail fast, not hang). + image: alpine/socat:latest + command: ["TCP-LISTEN:80,fork,reuseaddr", "SYSTEM:sleep 3600"] + seed: build: ./seed depends_on: @@ -82,7 +97,12 @@ services: dockerfile: docker/Dockerfile target: server environment: - UVICORN_NUM_WORKERS: "2" + # Single worker on purpose: the GitPolicyFetcher caches read by + # /internal/git-fetcher-cache-stats are per-process, so with >1 worker a + # round-robin read can miss the worker that fetched and make a `== 0` + # drain assertion pass falsely. One worker makes every cache read + # deterministic. The leak/boot/offline bugs all reproduce single-worker. + UVICORN_NUM_WORKERS: "1" OPAL_SCOPES: "1" OPAL_REDIS_URL: "redis://redis:6379" OPAL_BROADCAST_URI: "postgres://opal:opal@postgres:5432/opal" diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 4fefaf81d..3cb66a36a 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -14,9 +14,12 @@ GITEA_USER = "opaladmin" GITEA_PASSWORD = "opaladmin" -# TEST-NET-1 (RFC 5737): routable but runs no git server, so a clone hangs -# instead of failing fast — used to simulate an offline/unreachable repo. -UNREACHABLE_HOST = "192.0.2.1" +# the `blackhole` compose service (alpine/socat) accepts the TCP handshake then +# never answers, so a clone connects and blocks reading the response — a +# deterministic hang. Reachable from the opal_server container on the compose +# network. (A TEST-NET-1 address was rejected too fast on many networks, so the +# clone failed fast instead of hanging and the offline scenario wasn't exercised.) +UNREACHABLE_HOST = "blackhole" # the compose project lives next to this file; compose() runs from here _COMPOSE_DIR = str(Path(__file__).resolve().parent) @@ -47,16 +50,15 @@ def wait_healthy(self, timeout: int = 180) -> None: time.sleep(2) raise RuntimeError(f"opal-server not healthy in {timeout}s (last: {last})") - def stats(self, samples: int = 5, interval: float = 0.1) -> Dict[str, int]: - """Read the git-fetcher cache stats, merged across several reads. + def stats(self, samples: int = 3, interval: float = 0.1) -> Dict[str, int]: + """Read the git-fetcher cache stats, merged across a few reads. - The server runs multiple workers and the ``GitPolicyFetcher`` caches - are per-process, so a single read may land on a worker with empty - caches (e.g. a non-leader that never fetched). Sample a few times and - take the ``max`` per key, so the harness observes whichever worker - still holds the most entries — this prevents both false negatives - (missing a populated leader) and false positives (an ``== 0`` drain - assertion passing only because it hit an empty worker). + The stack runs a single uvicorn worker (see docker-compose.yml), so the + per-process ``GitPolicyFetcher`` caches are read deterministically — a + read can't miss the worker that fetched. Sampling a few times and taking + the ``max`` per key only smooths over a read that races an in-flight + mutation; it is not relied on to paper over multi-worker nondeterminism + (which the single-worker setup removes outright). """ merged: Dict[str, int] = {} for i in range(max(1, samples)): @@ -96,37 +98,75 @@ def delete_scope(self, scope_id: str) -> None: resp.raise_for_status() self._created_scopes.discard(scope_id) - def cleanup_created_scopes(self, drain_timeout: int = 15) -> None: - """Delete every scope created on this client, then best-effort wait for - the caches to drain — so the next test starts from a clean slate. - - Best-effort by design: on master, delete never purges the caches (the - leak this suite gates), so the drain wait will simply time out. A - teardown failure must never fail the test that just ran, hence the broad - excepts and the bounded wait. + def list_scope_ids(self) -> List[str]: + """All scope ids the server currently knows (GET /scopes).""" + resp = requests.get(f"{self.base_url}/scopes", timeout=30) + resp.raise_for_status() + return [s["scope_id"] for s in resp.json()] + + def hard_reset(self, timeout: int = 600) -> None: + """Recover the server from a saturated fetch executor by wiping state. + + When a test leaves many clones hung (the offline-repo test saturates the + executor on purpose), per-scope DELETEs would queue *behind* those hung + threads, and a plain restart would have ``preload_scopes`` re-clone the + offline scopes and saturate again. Instead: stop the server (killing the + hung threads), flush the Redis scope store so nothing is re-cloned, then + start clean. Used in that test's teardown so the session-scoped stack is + usable by every later test. """ - for scope_id in list(self._created_scopes): - try: - self.delete_scope(scope_id) - except Exception: - self._created_scopes.discard(scope_id) + compose("stop", "opal_server") + compose("exec", "-T", "redis", "redis-cli", "FLUSHALL") + compose("start", "opal_server") + self._created_scopes.clear() + self.wait_healthy(timeout=timeout) + + def delete_all_scopes(self, drain_timeout: int = 20) -> None: + """Delete every scope the *server* knows (not just this client's), then + best-effort wait for the caches to drain — a clean slate independent of + what any prior, possibly-failed, test left behind. + + Best-effort drain by design: on master, delete never purges the caches + (the leak this suite gates), so the wait simply times out. This runs in + fixture setup/teardown, so a failure here must not mask the test, hence + the broad excepts and bounded wait. + """ + try: + for scope_id in self.list_scope_ids(): + try: + self.delete_scope(scope_id) + except Exception: + self._created_scopes.discard(scope_id) + except Exception: + pass + self._created_scopes.clear() deadline = time.time() + drain_timeout while time.time() < deadline: try: - if self.stats(samples=1)["repos"] == 0: + if self.stats()["repo_locks"] == 0: return except Exception: return time.sleep(1) + def get_scope_policy(self, scope_id: str) -> requests.Response: + """Fetch a scope's policy bundle (GET /scopes/{id}/policy). + + A 200 here proves the scope's repo was cloned and is being served — + the signal that a healthy scope still works while another scope's + clone is hanging. + """ + return requests.get( + f"{self.base_url}/scopes/{scope_id}/policy", timeout=30 + ) + def refresh_all(self) -> None: - # Best-effort: POST /scopes/refresh publishes on the webhook topic so - # every leader re-syncs all scopes. If this OPAL build doesn't expose - # the route (404), treat it as a no-op — there is no client-side - # fallback; callers rely on their own stats polling either way. + # POST /scopes/refresh publishes on the webhook topic so the leader + # re-syncs all scopes. The second sync takes the discover/fetch path + # (not the first-sync clone path), which is what populates the `repos` + # and `repos_last_fetched` caches. A missing route is a real error and + # is surfaced via raise_for_status. resp = requests.post(f"{self.base_url}/scopes/refresh", timeout=30) - if resp.status_code == 404: - return resp.raise_for_status() @@ -203,32 +243,47 @@ def gitea_repo_url(name: str) -> str: def make_repo_unreachable(name: str) -> str: - """Return a git URL for ``name`` pointing at a routable-but-dead host. - - Simulates an offline/unreachable policy repo: the address is in - TEST-NET-1 (RFC 5737), which is routable but runs no git server, so a - clone hangs rather than failing fast — exercising the missing fetch - timeout on the scopes path (the bug PR3 fixes). The URL keeps the same - ``/{user}/{name}.git`` shape as a real Gitea repo so the scope looks - ordinary apart from the unreachable host. + """Return a git URL for ``name`` pointing at the ``blackhole`` sidecar. + + Simulates an offline/unreachable policy repo: ``blackhole`` (alpine/socat) + accepts the TCP handshake then never answers, so the clone connects and + blocks reading the git smart-HTTP response — a deterministic hang that + exercises the missing fetch timeout on the scopes path (the bug PR3 fixes). + The URL keeps the same ``/{user}/{name}.git`` shape as a real Gitea repo so + the scope looks ordinary apart from the unreachable host. """ return f"http://{UNREACHABLE_HOST}/{GITEA_USER}/{name}.git" def compose(*args: str) -> subprocess.CompletedProcess: - return subprocess.run( + """Run `docker compose `; on failure, surface the captured output. + + `capture_output=True` keeps compose noise out of passing tests, but a raw + CalledProcessError shows only the exit code — so on failure we re-raise + with the captured stdout/stderr embedded, otherwise a broken build/seed/ + restart is opaque to debug. + """ + proc = subprocess.run( ["docker", "compose", *args], cwd=_COMPOSE_DIR, capture_output=True, text=True, - check=True, ) + if proc.returncode != 0: + raise RuntimeError( + f"`docker compose {' '.join(args)}` failed (exit {proc.returncode})\n" + f"--- stdout ---\n{proc.stdout}\n--- stderr ---\n{proc.stderr}" + ) + return proc def bounce_postgres(down_seconds: int = 5) -> None: compose("stop", "postgres") time.sleep(down_seconds) - compose("start", "postgres") + # `up -d --wait` blocks until Postgres passes its healthcheck again (plain + # `compose start` has no --wait), so a recovery poll that follows isn't + # racing an unready broadcaster. --no-recreate keeps the same container. + compose("up", "-d", "--wait", "--no-recreate", "postgres") def list_seeded_repos(count: int) -> List[str]: diff --git a/app-tests/git-leak/seed/seed_gitea.py b/app-tests/git-leak/seed/seed_gitea.py index ddebe5667..0b8bf2e33 100644 --- a/app-tests/git-leak/seed/seed_gitea.py +++ b/app-tests/git-leak/seed/seed_gitea.py @@ -113,18 +113,36 @@ def main() -> int: token = _ensure_token(base_url, user, password) workdir = Path("/tmp/seed-work") + failures = [] for i in range(count): name = f"policy-repo-{i:04d}" - _ensure_repo(base_url, token, user, name) - # only push if the repo is empty (freshly created) - head = requests.get( - f"{base_url}/api/v1/repos/{user}/{name}/branches/main", - headers={"Authorization": f"token {token}"}, - timeout=10, + # Isolate per-repo failures: one bad push must not abort the loop and + # leave an indeterminate subset seeded. Collect failures and exit + # non-zero with a count so the harness sees a real seed error (and + # `docker compose wait seed` surfaces it) instead of a later, confusing + # load-gate timeout. + try: + _ensure_repo(base_url, token, user, name) + # only push if the repo is empty (freshly created) + head = requests.get( + f"{base_url}/api/v1/repos/{user}/{name}/branches/main", + headers={"Authorization": f"token {token}"}, + timeout=10, + ) + if head.status_code != 200: + _push_policy(base_url, token, user, name, workdir) + print(f"seeded {name}", flush=True) + except Exception as exc: # noqa: BLE001 - report, don't abort the loop + failures.append((name, repr(exc))) + print(f"FAILED {name}: {exc!r}", flush=True) + + if failures: + print( + f"ERROR: seeded {count - len(failures)}/{count} repos; " + f"{len(failures)} failed (e.g. {failures[:3]})", + flush=True, ) - if head.status_code != 200: - _push_policy(base_url, token, user, name, workdir) - print(f"seeded {name}", flush=True) + return 1 # write the token where the test harness can read it Path("/seed-output/token").write_text(token) diff --git a/app-tests/git-leak/test_boot.py b/app-tests/git-leak/test_boot.py index 928fd6f4e..ab9f1eb4b 100644 --- a/app-tests/git-leak/test_boot.py +++ b/app-tests/git-leak/test_boot.py @@ -17,14 +17,18 @@ def test_boot_loads_all_scopes(opal, repo_count): for i, name in enumerate(repos): opal.put_scope(f"boot-{i}", gitea_repo_url(name)) - # restart only the server so it re-runs sync_scopes on boot + # Start the clock at the restart, not after wait_healthy: preload_scopes() + # runs in gunicorn's `when_ready` (before workers accept traffic), so by the + # time /healthcheck answers, boot-sync may already be partly done — starting + # the clock later would undercount it. Measuring from the restart captures + # the whole boot-sync window. + start = time.time() compose("restart", "opal_server") opal.wait_healthy(timeout=600) - start = time.time() deadline = start + 2000 while time.time() < deadline: - if opal.stats()["repos"] >= n: + if opal.stats()["repo_locks"] >= n: break time.sleep(2) elapsed = time.time() - start @@ -32,5 +36,5 @@ def test_boot_loads_all_scopes(opal, repo_count): # PR1 records the baseline (loose). PR4 will set BOOT_TARGET_SECONDS low. BOOT_TARGET_SECONDS = int(os.environ.get("BOOT_TARGET_SECONDS", "2000")) print(f"boot loaded {n} scopes in {elapsed:.1f}s (target {BOOT_TARGET_SECONDS}s)") - assert opal.stats()["repos"] >= n, "not all scopes loaded after boot" + assert opal.stats()["repo_locks"] >= n, "not all scopes loaded after boot" assert elapsed < BOOT_TARGET_SECONDS, f"boot too slow: {elapsed:.1f}s" diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 8cfee6363..11b6da82d 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -13,52 +13,101 @@ def _wait_until(predicate, timeout=30, interval=0.5): return False +def _load_scopes(opal, prefix, names): + """PUT a scope per repo, then force a second sync so all three caches fill. + + The first sync of a fresh scope takes the clone path, which populates only + ``repo_locks`` — ``repos`` and ``repos_last_fetched`` are filled solely by + the discover/fetch path on a *subsequent* sync. ``refresh_all()`` triggers + that second sync, so after this returns all three caches reflect the N + scopes and the drain assertions test a cache the sync path actually fills. + + Returns the per-key load count reached (max over a wait). + """ + n = len(names) + for i, name in enumerate(names): + opal.put_scope(f"{prefix}-{i}", gitea_repo_url(name)) + # repo_locks is populated on the first sync (the clone path), so it is the + # deterministic signal that every scope was at least picked up. + locked = _wait_until(lambda: opal.stats()["repo_locks"] >= n, timeout=600) + assert locked, f"initial load never locked {n} repos: {opal.stats()}" + # force the discover/fetch path so `repos` / `repos_last_fetched` fill too + opal.refresh_all() + fetched = _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) + assert fetched, f"refresh never populated {n} repos: {opal.stats()}" + return opal.stats() + + @pytest.mark.timeout(900) def test_churn_releases_caches(opal, repo_count): """Create then delete many scopes; the three caches must return to empty. - FAILS on master: delete_scope never purges GitPolicyFetcher caches. + FAILS on this branch (without PR2): delete_scope never purges the + GitPolicyFetcher caches, so they stay populated after every scope is gone. + Becomes green once PR2 lands. """ n = min(repo_count, 100) repos = list_seeded_repos(n) - for i, name in enumerate(repos): - opal.put_scope(f"churn-{i}", gitea_repo_url(name)) - loaded = _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) - assert loaded, f"initial load never reached {n} repos: {opal.stats()}" + loaded = _load_scopes(opal, "churn", repos) + assert loaded["repo_locks"] >= n and loaded["repos"] >= n, loaded + rss_loaded = loaded["rss_kb"] for i in range(n): opal.delete_scope(f"churn-{i}") - released = _wait_until(lambda: opal.stats()["repos"] == 0, timeout=30) + # all three caches must drain to empty once every scope is deleted + released = _wait_until( + lambda: opal.stats()["repo_locks"] == 0 + and opal.stats()["repos"] == 0 + and opal.stats()["repos_last_fetched"] == 0, + timeout=60, + ) stats = opal.stats() - assert released, f"repos cache did not drain: {stats}" - assert stats["repos_last_fetched"] == 0, stats + assert released, f"caches did not drain after deleting all scopes: {stats}" + + # The cache drain above is the gate. RSS is only a loose backstop here: + # freeing the caches need not return memory to the OS (glibc/Python keep + # arenas), so this guards against a *gross* leak — RSS ballooning well past + # the loaded peak — without false-failing on allocator slack once PR2 lands. + rss_budget = rss_loaded + max(100_000, rss_loaded // 2) + assert stats["rss_kb"] <= rss_budget, ( + f"RSS ballooned across churn: {rss_loaded} -> {stats['rss_kb']} kb " + f"(budget {rss_budget})" + ) @pytest.mark.timeout(900) def test_repeat_sync_does_not_grow(opal, repo_count): - """Re-syncing the *same* scopes must not grow the caches. - - Idempotency guard, not a known-broken case. On current master this - PASSES: a clone path is keyed by the repo URL (``source_id`` = - sha256(url)+branch-shard), so re-syncing identical scopes reuses the - existing ``repos`` / ``repos_last_fetched`` entries instead of - allocating new ones. It guards against a regression that would make - repeat sync allocate per-sync. The unbounded *growth + no purge on - delete* leak is covered by ``test_churn_releases_caches`` above, which - uses distinct scopes and DOES fail on master. + """Re-syncing the *same* scopes must not grow memory unboundedly. + + Idempotency guard, not a known-broken case — PASSES on this branch. Clone + paths are keyed by the repo URL (``source_id`` = sha256(url)+branch-shard), + so re-syncing identical scopes reuses the existing ``repos`` / + ``repos_last_fetched`` entries rather than allocating new ones; the cache + *counts* therefore can't grow for any implementation, which is why the + load-bearing assertion here is on **RSS**, not on ``len(repos)``. It guards + against a regression where each repeat sync leaks per-sync allocations. The + unbounded growth-then-no-purge-on-delete leak is covered by + ``test_churn_releases_caches`` above, which uses distinct scopes. """ n = min(repo_count, 50) repos = list_seeded_repos(n) - for i, name in enumerate(repos): - opal.put_scope(f"stable-{i}", gitea_repo_url(name)) - loaded = _wait_until(lambda: opal.stats()["repos"] >= n, timeout=600) - assert loaded, f"initial sync never reached {n} repos: {opal.stats()}" + loaded = _load_scopes(opal, "stable", repos) + baseline_repos = loaded["repos"] + baseline_rss = loaded["rss_kb"] - baseline = opal.stats()["repos"] for _ in range(10): opal.refresh_all() time.sleep(2) - grown = opal.stats()["repos"] - assert grown <= baseline, f"repos cache grew on repeat sync: {baseline} -> {grown}" + grown = opal.stats() + assert grown["repos"] <= baseline_repos, ( + f"repos cache count grew on repeat sync: {baseline_repos} -> {grown['repos']}" + ) + # allow generous headroom for allocator slack; fail only on a real per-sync + # leak (10 refreshes of N scopes ballooning RSS). + rss_budget = baseline_rss + max(50_000, baseline_rss // 5) + assert grown["rss_kb"] <= rss_budget, ( + f"RSS grew unboundedly on repeat sync: " + f"{baseline_rss} -> {grown['rss_kb']} kb (budget {rss_budget})" + ) diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index b8fb73c89..11a25f90e 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -9,48 +9,83 @@ ) -@pytest.mark.timeout(300) +# Enough hanging clones to exhaust opal's default fetch executor +# (run_sync -> run_in_executor(None, ...), a ThreadPoolExecutor of +# min(32, cpu+4) workers). One hang wouldn't starve a multi-thread pool, so we +# saturate it with many; after PR3's fetch timeout these give up and free their +# threads, letting the healthy scope through. +OFFLINE_REPOS = 40 + + +@pytest.mark.timeout(420) def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): - """An unreachable repo must not stop healthy scopes from serving. + """Unreachable repos must not stop a healthy scope from serving. - FAILS on master: the scopes path has no fetch timeout, so a hung - clone occupies the shared executor and the server stalls. + FAILS on this branch (without PR3): the scopes path has no fetch timeout, + so the hung clones of the offline repos occupy the shared fetch executor + and the healthy scope's bundle never becomes available. """ - # a routable-but-dead address (TEST-NET-1, RFC 5737): the clone hangs - opal.put_scope("offline", make_repo_unreachable("dead-repo"), branch="main") + # the `blackhole` sidecar accepts the TCP handshake then never answers, so + # each of these clones hangs (holding a fetch-executor thread) rather than + # failing fast; enough of them saturate the pool. + for i in range(OFFLINE_REPOS): + opal.put_scope(f"offline-{i}", make_repo_unreachable(f"dead-{i}"), branch="main") healthy = list_seeded_repos(1)[0] opal.put_scope("healthy", gitea_repo_url(healthy)) - # the healthy scope's repo must appear in the cache within a bounded time, - # even though the offline scope is hanging - deadline = time.time() + 60 - served = False - while time.time() < deadline: - if opal.stats()["repos"] >= 1: - served = True - break - time.sleep(2) - assert served, "healthy scope never loaded while an offline repo was hanging" + try: + # The healthy scope must become *servable* within a bounded time even + # while the offline scopes hang. A 200 from its policy bundle proves the + # clone completed and the scope is served — a stronger signal than a + # cache count, and exactly what the offline hang starves on master. + deadline = time.time() + 90 + served = False + last = None + while time.time() < deadline: + try: + resp = opal.get_scope_policy("healthy") + last = resp.status_code + if resp.status_code == 200: + served = True + break + except Exception as exc: # request itself may stall when starved + last = repr(exc) + time.sleep(2) + assert served, ( + f"healthy scope never served while {OFFLINE_REPOS} offline repos " + f"were hanging (last policy response: {last})" + ) + finally: + # The offline clones hang for the blackhole's full duration, occupying + # executor threads. On the session-scoped stack that would starve every + # later test, and per-scope DELETEs would queue behind the hung threads. + # hard_reset stops the server (killing the hung threads), flushes the + # Redis scope store so preload doesn't re-clone them, and restarts clean. + opal.hard_reset() @pytest.mark.timeout(300) -def test_server_recovers_after_postgres_bounce(opal): - """A transient Postgres (broadcaster) outage must not leave the server - down. - - Recovery guard, not a known-broken case. On current master this - PASSES: when the broadcast channel drops, the affected worker - triggers a graceful shutdown and gunicorn respawns it, while the - sibling worker keeps serving HTTP — so the surface recovers within - the window. It guards against a regression of that property - (PER-15065's in-process reconnect would make recovery cleaner by - avoiding the worker churn, but recovery already holds). +def test_server_recovers_after_postgres_bounce(opal, repo_count): + """A transient Postgres (broadcaster) outage must not break propagation. + + Recovery guard, not a known-broken case — PASSES on this branch: when the + broadcast channel drops, the affected worker triggers a graceful shutdown + and gunicorn respawns it, and once Postgres is back the broadcaster + reconnects. We prove recovery of the *broadcast path*, not just HTTP + liveness: after the bounce we PUT a fresh scope and assert it actually + syncs (its repo lands in the cache), which only happens if the leader + received the sync notification over the recovered broadcaster. + (PER-15065's in-process reconnect would make recovery cleaner by avoiding + the worker churn, but recovery already holds.) """ assert opal.stats() # healthy before + baseline_locks = opal.stats()["repo_locks"] + bounce_postgres(down_seconds=5) - deadline = time.time() + 60 + # wait for the HTTP surface to come back first + deadline = time.time() + 90 recovered = False while time.time() < deadline: try: @@ -60,4 +95,16 @@ def test_server_recovers_after_postgres_bounce(opal): break except Exception: time.sleep(2) - assert recovered, "server did not recover within 60s of a postgres bounce" + assert recovered, "server did not recover HTTP within 90s of a postgres bounce" + + # prove the broadcast path itself recovered: a freshly PUT scope must sync + healthy = list_seeded_repos(1)[0] + opal.put_scope("post-bounce", gitea_repo_url(healthy)) + synced = False + deadline = time.time() + 120 + while time.time() < deadline: + if opal.stats()["repo_locks"] > baseline_locks: + synced = True + break + time.sleep(2) + assert synced, "scope PUT after the bounce never synced; broadcaster did not recover" diff --git a/packages/opal-server/opal_server/debug_stats.py b/packages/opal-server/opal_server/debug_stats.py index 20ea6b341..af77ff4ea 100644 --- a/packages/opal-server/opal_server/debug_stats.py +++ b/packages/opal-server/opal_server/debug_stats.py @@ -4,9 +4,9 @@ observe the cache growth that the memory-leak fix (PR2) eliminates. """ from pathlib import Path -from typing import Dict +from typing import Dict, List, Optional -from fastapi import FastAPI +from fastapi import FastAPI, params from opal_server.git_fetcher import GitPolicyFetcher @@ -31,11 +31,26 @@ def git_fetcher_cache_stats() -> Dict[str, int]: } -def register_internal_stats_route(app: FastAPI, enabled: bool) -> None: - """Mount GET /internal/git-fetcher-cache-stats only when enabled.""" +def register_internal_stats_route( + app: FastAPI, + enabled: bool, + dependencies: Optional[List[params.Depends]] = None, +) -> None: + """Mount GET /internal/git-fetcher-cache-stats only when enabled. + + ``dependencies`` are applied to the route (e.g. the server's + ``JWTAuthenticator``) so the endpoint is protected when JWT verification + is enabled. When verification is disabled — as in the test bed, which + leaves ``OPAL_AUTH_PUBLIC_KEY`` unset — the authenticator is a no-op and + the route stays reachable without a token. + """ if not enabled: return - @app.get("/internal/git-fetcher-cache-stats", include_in_schema=False) + @app.get( + "/internal/git-fetcher-cache-stats", + include_in_schema=False, + dependencies=dependencies or [], + ) def _git_fetcher_cache_stats() -> Dict[str, int]: return git_fetcher_cache_stats() diff --git a/packages/opal-server/opal_server/server.py b/packages/opal-server/opal_server/server.py index 1be796d6d..9702e372f 100644 --- a/packages/opal-server/opal_server/server.py +++ b/packages/opal-server/opal_server/server.py @@ -295,7 +295,9 @@ def healthcheck(): return {"status": "ok"} register_internal_stats_route( - app, enabled=opal_server_config.DEBUG_INTERNAL_STATS + app, + enabled=opal_server_config.DEBUG_INTERNAL_STATS, + dependencies=[Depends(authenticator)], ) return app diff --git a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py index 00a9c8e1b..3e36cd330 100644 --- a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py +++ b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py @@ -1,4 +1,4 @@ -from fastapi import FastAPI +from fastapi import Depends, FastAPI, HTTPException from fastapi.testclient import TestClient from opal_server.debug_stats import register_internal_stats_route @@ -20,3 +20,20 @@ def test_endpoint_present_when_enabled(): assert resp.status_code == 200 body = resp.json() assert set(body) == {"repo_locks", "repos", "repos_last_fetched", "rss_kb"} + + +def test_endpoint_applies_passed_dependencies(): + """A route dependency (e.g. the server's authenticator) is enforced. + + Mirrors how server.py wires the real JWTAuthenticator: when verification is + enabled the dependency rejects unauthenticated reads; when disabled it is a + no-op (covered by the test above, which passes no dependency). + """ + + def _deny(): + raise HTTPException(status_code=401, detail="unauthorized") + + app = FastAPI() + register_internal_stats_route(app, enabled=True, dependencies=[Depends(_deny)]) + resp = TestClient(app).get("/internal/git-fetcher-cache-stats") + assert resp.status_code == 401 From f810db8faf7f8ab953f179c956cd51f17852ff47 Mon Sep 17 00:00:00 2001 From: David Shoen Date: Wed, 24 Jun 2026 16:15:05 +0300 Subject: [PATCH 10/17] style(git-leak): apply black/isort/docformatter (pre-commit) Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/helpers.py | 18 ++++++++---------- app-tests/git-leak/test_leak.py | 10 +++++----- app-tests/git-leak/test_resilience.py | 16 ++++++++++------ .../tests/debug_stats_endpoint_test.py | 7 ++++--- 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 3cb66a36a..3dc549421 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -152,13 +152,11 @@ def delete_all_scopes(self, drain_timeout: int = 20) -> None: def get_scope_policy(self, scope_id: str) -> requests.Response: """Fetch a scope's policy bundle (GET /scopes/{id}/policy). - A 200 here proves the scope's repo was cloned and is being served — - the signal that a healthy scope still works while another scope's - clone is hanging. + A 200 here proves the scope's repo was cloned and is being + served — the signal that a healthy scope still works while + another scope's clone is hanging. """ - return requests.get( - f"{self.base_url}/scopes/{scope_id}/policy", timeout=30 - ) + return requests.get(f"{self.base_url}/scopes/{scope_id}/policy", timeout=30) def refresh_all(self) -> None: # POST /scopes/refresh publishes on the webhook topic so the leader @@ -258,10 +256,10 @@ def make_repo_unreachable(name: str) -> str: def compose(*args: str) -> subprocess.CompletedProcess: """Run `docker compose `; on failure, surface the captured output. - `capture_output=True` keeps compose noise out of passing tests, but a raw - CalledProcessError shows only the exit code — so on failure we re-raise - with the captured stdout/stderr embedded, otherwise a broken build/seed/ - restart is opaque to debug. + `capture_output=True` keeps compose noise out of passing tests, but + a raw CalledProcessError shows only the exit code — so on failure we + re-raise with the captured stdout/stderr embedded, otherwise a + broken build/seed/ restart is opaque to debug. """ proc = subprocess.run( ["docker", "compose", *args], diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 11b6da82d..7ded9eded 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -43,8 +43,8 @@ def test_churn_releases_caches(opal, repo_count): """Create then delete many scopes; the three caches must return to empty. FAILS on this branch (without PR2): delete_scope never purges the - GitPolicyFetcher caches, so they stay populated after every scope is gone. - Becomes green once PR2 lands. + GitPolicyFetcher caches, so they stay populated after every scope is + gone. Becomes green once PR2 lands. """ n = min(repo_count, 100) repos = list_seeded_repos(n) @@ -101,9 +101,9 @@ def test_repeat_sync_does_not_grow(opal, repo_count): time.sleep(2) grown = opal.stats() - assert grown["repos"] <= baseline_repos, ( - f"repos cache count grew on repeat sync: {baseline_repos} -> {grown['repos']}" - ) + assert ( + grown["repos"] <= baseline_repos + ), f"repos cache count grew on repeat sync: {baseline_repos} -> {grown['repos']}" # allow generous headroom for allocator slack; fail only on a real per-sync # leak (10 refreshes of N scopes ballooning RSS). rss_budget = baseline_rss + max(50_000, baseline_rss // 5) diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index 11a25f90e..76515dbe0 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -8,7 +8,6 @@ make_repo_unreachable, ) - # Enough hanging clones to exhaust opal's default fetch executor # (run_sync -> run_in_executor(None, ...), a ThreadPoolExecutor of # min(32, cpu+4) workers). One hang wouldn't starve a multi-thread pool, so we @@ -21,15 +20,18 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): """Unreachable repos must not stop a healthy scope from serving. - FAILS on this branch (without PR3): the scopes path has no fetch timeout, - so the hung clones of the offline repos occupy the shared fetch executor - and the healthy scope's bundle never becomes available. + FAILS on this branch (without PR3): the scopes path has no fetch + timeout, so the hung clones of the offline repos occupy the shared + fetch executor and the healthy scope's bundle never becomes + available. """ # the `blackhole` sidecar accepts the TCP handshake then never answers, so # each of these clones hangs (holding a fetch-executor thread) rather than # failing fast; enough of them saturate the pool. for i in range(OFFLINE_REPOS): - opal.put_scope(f"offline-{i}", make_repo_unreachable(f"dead-{i}"), branch="main") + opal.put_scope( + f"offline-{i}", make_repo_unreachable(f"dead-{i}"), branch="main" + ) healthy = list_seeded_repos(1)[0] opal.put_scope("healthy", gitea_repo_url(healthy)) @@ -107,4 +109,6 @@ def test_server_recovers_after_postgres_bounce(opal, repo_count): synced = True break time.sleep(2) - assert synced, "scope PUT after the bounce never synced; broadcaster did not recover" + assert ( + synced + ), "scope PUT after the bounce never synced; broadcaster did not recover" diff --git a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py index 3e36cd330..4abd1abb1 100644 --- a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py +++ b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py @@ -25,9 +25,10 @@ def test_endpoint_present_when_enabled(): def test_endpoint_applies_passed_dependencies(): """A route dependency (e.g. the server's authenticator) is enforced. - Mirrors how server.py wires the real JWTAuthenticator: when verification is - enabled the dependency rejects unauthenticated reads; when disabled it is a - no-op (covered by the test above, which passes no dependency). + Mirrors how server.py wires the real JWTAuthenticator: when + verification is enabled the dependency rejects unauthenticated + reads; when disabled it is a no-op (covered by the test above, which + passes no dependency). """ def _deny(): From 82ff33a7300adfe857c115d64ccea5ec135ee2be Mon Sep 17 00:00:00 2001 From: David Shoen Date: Wed, 24 Jun 2026 19:57:23 +0300 Subject: [PATCH 11/17] test(git-leak): tighten stat polling and pin test-bed images (PR review) - snapshot a single /internal stats read per poll in the churn-drain and boot gates (consistent multi-key observation; fewer HTTP round-trips) - document why a 200 from the healthy scope can't be a masked default bundle, and why the stats route is intentionally a sync def - pin alpine/socat and the seed image's pip deps for reproducibility Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/docker-compose.yml | 2 +- app-tests/git-leak/seed/Dockerfile | 2 +- app-tests/git-leak/test_boot.py | 6 ++++-- app-tests/git-leak/test_leak.py | 15 ++++++++------- app-tests/git-leak/test_resilience.py | 5 +++++ packages/opal-server/opal_server/debug_stats.py | 5 +++++ 6 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app-tests/git-leak/docker-compose.yml b/app-tests/git-leak/docker-compose.yml index bd48c26dc..a863faed4 100644 --- a/app-tests/git-leak/docker-compose.yml +++ b/app-tests/git-leak/docker-compose.yml @@ -72,7 +72,7 @@ services: # blocks reading the git smart-HTTP response, holding the fetch executor. # Deterministic, unlike a TEST-NET-1 address which many networks reject # fast with ICMP-unreachable (so the clone would fail fast, not hang). - image: alpine/socat:latest + image: alpine/socat:1.8.0.3 command: ["TCP-LISTEN:80,fork,reuseaddr", "SYSTEM:sleep 3600"] seed: diff --git a/app-tests/git-leak/seed/Dockerfile b/app-tests/git-leak/seed/Dockerfile index ee9d776d2..540118cd2 100644 --- a/app-tests/git-leak/seed/Dockerfile +++ b/app-tests/git-leak/seed/Dockerfile @@ -1,6 +1,6 @@ FROM python:3.11-slim RUN apt-get update && apt-get install -y --no-install-recommends git \ && rm -rf /var/lib/apt/lists/* -RUN pip install --no-cache-dir requests GitPython +RUN pip install --no-cache-dir requests==2.32.3 GitPython==3.1.50 COPY seed_gitea.py /seed_gitea.py ENTRYPOINT ["python", "/seed_gitea.py"] diff --git a/app-tests/git-leak/test_boot.py b/app-tests/git-leak/test_boot.py index ab9f1eb4b..b28637ec1 100644 --- a/app-tests/git-leak/test_boot.py +++ b/app-tests/git-leak/test_boot.py @@ -27,8 +27,10 @@ def test_boot_loads_all_scopes(opal, repo_count): opal.wait_healthy(timeout=600) deadline = start + 2000 + loaded = 0 while time.time() < deadline: - if opal.stats()["repo_locks"] >= n: + loaded = opal.stats()["repo_locks"] + if loaded >= n: break time.sleep(2) elapsed = time.time() - start @@ -36,5 +38,5 @@ def test_boot_loads_all_scopes(opal, repo_count): # PR1 records the baseline (loose). PR4 will set BOOT_TARGET_SECONDS low. BOOT_TARGET_SECONDS = int(os.environ.get("BOOT_TARGET_SECONDS", "2000")) print(f"boot loaded {n} scopes in {elapsed:.1f}s (target {BOOT_TARGET_SECONDS}s)") - assert opal.stats()["repo_locks"] >= n, "not all scopes loaded after boot" + assert loaded >= n, "not all scopes loaded after boot" assert elapsed < BOOT_TARGET_SECONDS, f"boot too slow: {elapsed:.1f}s" diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 7ded9eded..8ef2258f7 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -55,13 +55,14 @@ def test_churn_releases_caches(opal, repo_count): for i in range(n): opal.delete_scope(f"churn-{i}") - # all three caches must drain to empty once every scope is deleted - released = _wait_until( - lambda: opal.stats()["repo_locks"] == 0 - and opal.stats()["repos"] == 0 - and opal.stats()["repos_last_fetched"] == 0, - timeout=60, - ) + # all three caches must drain to empty once every scope is deleted. Read a + # single stats snapshot per poll so the three keys reflect the same + # observation (and to avoid 3x the HTTP round-trips per iteration). + def _all_caches_empty() -> bool: + s = opal.stats() + return s["repo_locks"] == 0 and s["repos"] == 0 and s["repos_last_fetched"] == 0 + + released = _wait_until(_all_caches_empty, timeout=60) stats = opal.stats() assert released, f"caches did not drain after deleting all scopes: {stats}" diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index 76515dbe0..b18829e12 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -41,6 +41,11 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): # while the offline scopes hang. A 200 from its policy bundle proves the # clone completed and the scope is served — a stronger signal than a # cache count, and exactly what the offline hang starves on master. + # + # A 200 here can't be a *masked* default bundle: GET /{scope}/policy + # falls back to the "default" scope on a bad/missing repo, but this bed + # never creates a "default" scope, so that fallback raises instead of + # returning 200. The only way to get 200 is the healthy clone completing. deadline = time.time() + 90 served = False last = None diff --git a/packages/opal-server/opal_server/debug_stats.py b/packages/opal-server/opal_server/debug_stats.py index af77ff4ea..5439ce950 100644 --- a/packages/opal-server/opal_server/debug_stats.py +++ b/packages/opal-server/opal_server/debug_stats.py @@ -47,6 +47,11 @@ def register_internal_stats_route( if not enabled: return + # Deliberately a sync def: Starlette runs it in its own threadpool, which is + # independent of the default loop executor opal uses for git fetches + # (run_sync -> run_in_executor(None, ...)). So this endpoint keeps answering + # even when hung clones saturate the fetch executor — which is exactly the + # condition the offline-repo test observes through it. @app.get( "/internal/git-fetcher-cache-stats", include_in_schema=False, From 75ad43a4a90ac23843d0e0e68f2775d3d717584c Mon Sep 17 00:00:00 2001 From: David Shoen Date: Sun, 28 Jun 2026 18:45:58 +0300 Subject: [PATCH 12/17] test(git-leak): isolate offline-hang healthy probe to a never-cloned repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ziv (PR review, round 2) caught that the offline-resilience gate can false-PASS in the full-suite run. The "healthy" scope pointed at policy-repo-0000 (list_seeded_repos(1)[0]), but on-disk clones are keyed by URL-hash and survive compose restart/stop/start (opal_server mounts no volume at /opal; only `down -v` wipes them). test_boot/test_leak run first (alphabetical) and already clone every seeded repo, so the healthy scope hit the existing clone via _discover_repository, skipped _clone(), and served 200 without ever touching the saturated fetch executor — the gate that must FAIL on this branch (no PR3 timeout) passed. Fix: seed a reserved repo (policy-repo-healthy-probe) outside the numeric policy-repo-NNNN range that no boot/leak test enumerates, and point the healthy probe at it. A never-cloned repo forces a genuine fresh clone through the starved pool, so the gate fails correctly. The seed- completeness check in conftest now covers the reserved repo too. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/conftest.py | 12 ++++++++++-- app-tests/git-leak/helpers.py | 14 ++++++++++++++ app-tests/git-leak/seed/seed_gitea.py | 19 +++++++++++++++---- app-tests/git-leak/test_resilience.py | 11 +++++++++-- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index 1660d596b..446376e3f 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -2,7 +2,13 @@ import shutil import pytest -from helpers import GiteaAdmin, OpalServerClient, compose, list_seeded_repos +from helpers import ( + HEALTHY_PROBE_REPO, + GiteaAdmin, + OpalServerClient, + compose, + list_seeded_repos, +) def pytest_addoption(parser): @@ -43,7 +49,9 @@ def stack(request, repo_count): # Verify the seed actually produced all N repos before any test runs: a # partial seed would otherwise look like a server bug when the load gate # can't reach N. Fail loudly with the gap. - expected = set(list_seeded_repos(repo_count)) + # include the reserved probe repo the resilience test relies on, so a + # partial seed of it is caught here too rather than as a later test failure + expected = set(list_seeded_repos(repo_count)) | {HEALTHY_PROBE_REPO} present = set(GiteaAdmin().list_repos()) missing = expected - present assert not missing, ( diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 3dc549421..f819d0d01 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -286,3 +286,17 @@ def bounce_postgres(down_seconds: int = 5) -> None: def list_seeded_repos(count: int) -> List[str]: return [f"policy-repo-{i:04d}" for i in range(count)] + + +# A reserved repo seeded *outside* the numeric ``policy-repo-NNNN`` range that +# ``list_seeded_repos`` enumerates, so no boot/leak test ever clones it. The +# resilience offline-hang test uses it as its "healthy" probe: clones live at +# ``base_dir/`` keyed by URL-hash and survive ``compose +# restart/stop/start`` (opal_server mounts no volume at ``/opal``; only +# ``down -v`` wipes them), so pointing the probe at any shared seeded repo would +# let the healthy scope reuse an on-disk clone and serve 200 *without* touching +# the saturated fetch executor — false-passing a gate that must FAIL on this +# branch. A dedicated never-cloned repo forces a genuine fresh clone through the +# starved executor. Keep this name in sync with ``RESERVED_REPOS`` in +# ``seed/seed_gitea.py``. +HEALTHY_PROBE_REPO = "policy-repo-healthy-probe" diff --git a/app-tests/git-leak/seed/seed_gitea.py b/app-tests/git-leak/seed/seed_gitea.py index 0b8bf2e33..7e1de1a4c 100644 --- a/app-tests/git-leak/seed/seed_gitea.py +++ b/app-tests/git-leak/seed/seed_gitea.py @@ -28,6 +28,14 @@ DATA_JSON = '{"roles": {"admin": ["read", "write"]}}\n' +# Reserved repos seeded in addition to the numeric ``policy-repo-NNNN`` set. +# These sit outside the range the boot/leak tests enumerate (so no test ever +# clones them) and back the resilience offline-hang test's "healthy" probe, +# which must force a fresh clone through the saturated executor rather than +# reuse a surviving on-disk clone. Keep in sync with ``HEALTHY_PROBE_REPO`` in +# ``helpers.py``. +RESERVED_REPOS = ("policy-repo-healthy-probe",) + def _wait_for_gitea(base_url: str, timeout: int = 120) -> None: deadline = time.time() + timeout @@ -114,8 +122,10 @@ def main() -> int: workdir = Path("/tmp/seed-work") failures = [] - for i in range(count): - name = f"policy-repo-{i:04d}" + # the numeric set the boot/leak tests enumerate, plus the reserved repos + # (e.g. the resilience offline-hang test's never-cloned healthy probe) + names = [f"policy-repo-{i:04d}" for i in range(count)] + list(RESERVED_REPOS) + for name in names: # Isolate per-repo failures: one bad push must not abort the loop and # leave an indeterminate subset seeded. Collect failures and exit # non-zero with a count so the harness sees a real seed error (and @@ -136,9 +146,10 @@ def main() -> int: failures.append((name, repr(exc))) print(f"FAILED {name}: {exc!r}", flush=True) + total = len(names) if failures: print( - f"ERROR: seeded {count - len(failures)}/{count} repos; " + f"ERROR: seeded {total - len(failures)}/{total} repos; " f"{len(failures)} failed (e.g. {failures[:3]})", flush=True, ) @@ -146,7 +157,7 @@ def main() -> int: # write the token where the test harness can read it Path("/seed-output/token").write_text(token) - print(f"DONE: ensured {count} repos", flush=True) + print(f"DONE: ensured {total} repos", flush=True) return 0 diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index b18829e12..60f28338f 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -2,6 +2,7 @@ import pytest from helpers import ( + HEALTHY_PROBE_REPO, bounce_postgres, gitea_repo_url, list_seeded_repos, @@ -33,8 +34,14 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): f"offline-{i}", make_repo_unreachable(f"dead-{i}"), branch="main" ) - healthy = list_seeded_repos(1)[0] - opal.put_scope("healthy", gitea_repo_url(healthy)) + # Point the healthy scope at a repo *no other test clones* (HEALTHY_PROBE_REPO + # is seeded outside the numeric range the boot/leak tests enumerate). Clones + # survive compose restart/stop/start, so reusing a shared seeded repo here + # would let this scope hit the existing on-disk clone via _discover_repository, + # skip _clone(), and serve 200 without ever touching the saturated executor — + # false-passing this gate. A never-cloned repo forces a real clone through the + # starved pool, so the gate fails correctly on this branch (no PR3 timeout). + opal.put_scope("healthy", gitea_repo_url(HEALTHY_PROBE_REPO)) try: # The healthy scope must become *servable* within a bounded time even From 15f3cfe2495735936652400edd6ea8fe3fe8bb77 Mon Sep 17 00:00:00 2001 From: David Shoen Date: Sun, 28 Jun 2026 19:05:33 +0300 Subject: [PATCH 13/17] test(git-leak): harden harness teardown and tighten assertions (PR review) Follow-up to PR review of the git-leak/resilience test bed: - hard_reset: always restart opal_server + wait_healthy via a finally, so a failed redis FLUSHALL can't leave the server stopped (which would fail every later session-scoped test and, running in a test finally, mask the result). - delete_all_scopes drain: a transient /internal read error no longer counts as a successful drain (was `except: return`); keep polling to the deadline so a not-yet-drained cache can't leak into the next test once PR2 lands. - use stats(samples=1) for the zero-waiting drain/empty polls (the peak-merge only matters for load assertions; this also drops 3x HTTP per poll). - resilience: narrow broad `except Exception` to requests.RequestException (and RuntimeError for wait_healthy timeout) so harness bugs surface instead of masquerading as "never served"/"never recovered". - resilience: collapse `assert opal.stats()` + redundant re-read into one read. - debug_stats_test: assert rss_kb > 0 on Linux (was `>= 0`, which passed for the wrong reason where /proc is absent and RSS reads fall back to 0). Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/helpers.py | 22 ++++++++++++++----- app-tests/git-leak/test_leak.py | 2 +- app-tests/git-leak/test_resilience.py | 11 ++++++---- .../opal_server/tests/debug_stats_test.py | 9 +++++++- 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index f819d0d01..452d16b99 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -116,10 +116,15 @@ def hard_reset(self, timeout: int = 600) -> None: usable by every later test. """ compose("stop", "opal_server") - compose("exec", "-T", "redis", "redis-cli", "FLUSHALL") - compose("start", "opal_server") - self._created_scopes.clear() - self.wait_healthy(timeout=timeout) + try: + compose("exec", "-T", "redis", "redis-cli", "FLUSHALL") + finally: + # Always bring the server back up, even if the flush failed: leaving + # it stopped would fail every later session-scoped test, and since + # this runs in a test's `finally` it would also mask the real result. + compose("start", "opal_server") + self._created_scopes.clear() + self.wait_healthy(timeout=timeout) def delete_all_scopes(self, drain_timeout: int = 20) -> None: """Delete every scope the *server* knows (not just this client's), then @@ -143,10 +148,15 @@ def delete_all_scopes(self, drain_timeout: int = 20) -> None: deadline = time.time() + drain_timeout while time.time() < deadline: try: - if self.stats()["repo_locks"] == 0: + # Single snapshot: we're waiting for zero, so the peak-merge + # (max over samples) would only delay observing the drain. + if self.stats(samples=1)["repo_locks"] == 0: return except Exception: - return + # A transient stats-read failure is not proof of a drain — keep + # polling until the deadline rather than returning early, which + # would let a not-yet-drained cache leak into the next test. + pass time.sleep(1) def get_scope_policy(self, scope_id: str) -> requests.Response: diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 8ef2258f7..09fe9034d 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -59,7 +59,7 @@ def test_churn_releases_caches(opal, repo_count): # single stats snapshot per poll so the three keys reflect the same # observation (and to avoid 3x the HTTP round-trips per iteration). def _all_caches_empty() -> bool: - s = opal.stats() + s = opal.stats(samples=1) return s["repo_locks"] == 0 and s["repos"] == 0 and s["repos_last_fetched"] == 0 released = _wait_until(_all_caches_empty, timeout=60) diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index 60f28338f..04702448b 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -1,6 +1,7 @@ import time import pytest +import requests from helpers import ( HEALTHY_PROBE_REPO, bounce_postgres, @@ -63,7 +64,7 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): if resp.status_code == 200: served = True break - except Exception as exc: # request itself may stall when starved + except requests.RequestException as exc: # may stall when starved last = repr(exc) time.sleep(2) assert served, ( @@ -93,8 +94,9 @@ def test_server_recovers_after_postgres_bounce(opal, repo_count): (PER-15065's in-process reconnect would make recovery cleaner by avoiding the worker churn, but recovery already holds.) """ - assert opal.stats() # healthy before - baseline_locks = opal.stats()["repo_locks"] + baseline = opal.stats() # healthy before + assert baseline + baseline_locks = baseline["repo_locks"] bounce_postgres(down_seconds=5) @@ -107,7 +109,8 @@ def test_server_recovers_after_postgres_bounce(opal, repo_count): opal.stats() recovered = True break - except Exception: + except (requests.RequestException, RuntimeError): + # RequestException: HTTP not back yet; RuntimeError: wait_healthy timed out time.sleep(2) assert recovered, "server did not recover HTTP within 90s of a postgres bounce" diff --git a/packages/opal-server/opal_server/tests/debug_stats_test.py b/packages/opal-server/opal_server/tests/debug_stats_test.py index b2d7935db..c67fb3530 100644 --- a/packages/opal-server/opal_server/tests/debug_stats_test.py +++ b/packages/opal-server/opal_server/tests/debug_stats_test.py @@ -1,3 +1,5 @@ +import sys + from opal_server.config import opal_server_config from opal_server.debug_stats import git_fetcher_cache_stats from opal_server.git_fetcher import GitPolicyFetcher @@ -14,7 +16,12 @@ def test_stats_report_dict_sizes(monkeypatch): assert stats["repos"] == 2 assert stats["repos_last_fetched"] == 0 assert isinstance(stats["rss_kb"], int) - assert stats["rss_kb"] >= 0 + # On Linux /proc/self/status exists, so RSS reading must actually work; on + # other platforms _read_rss_kb falls back to 0 and the wiring is untestable. + if sys.platform.startswith("linux"): + assert stats["rss_kb"] > 0 + else: + assert stats["rss_kb"] >= 0 def test_internal_stats_flag_defaults_off(): From 8e24cb0b5479f2974b7c9dd5a7c92ac66f07e78c Mon Sep 17 00:00:00 2001 From: David Shoen Date: Wed, 1 Jul 2026 12:33:11 +0300 Subject: [PATCH 14/17] test(git-leak): make PR4/PR5 tests genuine gates (address PR review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zeev's gate-coverage review found only 2 of 5 flagship tests (churn #1, offline #4) were genuine fail-now / pass-after gates. Lift the other three: - #5 broadcaster: run 2 workers (OPAL_TEST_WORKERS) so the Postgres backbone is actually fanned out cross-worker (references/debug-pubsub.md §3-4), and assert the gunicorn worker PIDs are unchanged across a transient bounce -- the in-place-reconnect signal that distinguishes #915 (PER-15065) from a plain worker respawn. Prove recovery via a servable post-bounce scope, not /internal cache counts (per-process, non-deterministic on 2 workers). - #3 boot: key completion on "all scopes served" (GET /scopes/{id}/policy == 200) instead of repo_locks (set at fetch start, so it undercounts the final clone); document the PR4 tight-BOOT_TARGET_SECONDS carry-forward. - #2 repeat-sync: rename to test_repeat_sync_rss_stays_bounded and drop the tautological len(repos) assertion; RSS is the sole (load-bearing) gate. Adds worker_pids() (/proc-based, matched host-side so the scan can't count its own sh -c wrapper) and the opal_multiworker fixture (recreate to 2 workers, restore to 1 on teardown). Validated live (--boot-scopes=50): #2/#3 pass, #5 passes (worker PIDs held across the bounce, post-bounce scope served), #1/#4 fail for the right reason (PR2/PR3 not landed). Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/README.md | 37 ++++++++----- app-tests/git-leak/conftest.py | 32 +++++++++++ app-tests/git-leak/docker-compose.yml | 14 ++++- app-tests/git-leak/helpers.py | 44 +++++++++++++++ app-tests/git-leak/test_boot.py | 55 ++++++++++++++----- app-tests/git-leak/test_leak.py | 30 +++++----- app-tests/git-leak/test_resilience.py | 79 ++++++++++++++++++--------- 7 files changed, 220 insertions(+), 71 deletions(-) diff --git a/app-tests/git-leak/README.md b/app-tests/git-leak/README.md index aabbb4657..4b5d9c486 100644 --- a/app-tests/git-leak/README.md +++ b/app-tests/git-leak/README.md @@ -37,20 +37,31 @@ Useful flags: `--boot-scopes=N` (any N), `--keep-stack` (skip teardown), env `BOOT_TARGET_SECONDS=120` (tighten the boot gate). ## Expected behavior -The churn leak test (`test_churn_releases_caches`) and the offline-repo test -FAIL on this branch *without the PR2/PR3 fix* — they target unfixed bugs and -become the regression gates for PR2/PR3, flipping green when those land. The -boot test passes but fails when `BOOT_TARGET_SECONDS` is set low (PR4's gate). -Two tests are guards that PASS rather than reproducing a current failure: -- `test_repeat_sync_does_not_grow` — clone paths are keyed by the repo URL, so - re-syncing identical scopes reuses cache entries and the cache *counts* can't - grow for any implementation; the load-bearing assertion is therefore on RSS, - guarding against a regression that leaks per-sync allocations. -- `test_server_recovers_after_postgres_bounce` — when the broadcaster drops, the - worker is respawned by gunicorn and the broadcaster reconnects once Postgres - is back; the test PUTs a fresh scope post-bounce and asserts it syncs, proving - the broadcast path (not just HTTP) recovered. +Gate-coverage matrix (what each flagship test actually does): + +| Test | Role | Behaviour here | +|---|---|---| +| `test_churn_releases_caches` | **gate (PR2)** | FAILS without the PR2 leak fix — delete leaves the caches populated; flips green when PR2 lands | +| `test_offline_repo_does_not_block_healthy_scopes` | **gate (PR3)** | FAILS without the PR3 fetch timeout — 40 hung clones starve the executor so a healthy scope never serves; flips green when PR3 lands | +| `test_boot_loads_all_scopes` | **baseline → gate (PR4)** | PASSES with the loose default target; set `BOOT_TARGET_SECONDS` low (plan: 120 @ 50) on PR4 to gate the parallel-boot fix | +| `test_repeat_sync_rss_stays_bounded` | **RSS guard** | PASSES; an RSS-budget guard against per-sync allocation leaks (the cache *count* can't grow for any impl, so there is no count assertion — see below) | +| `test_server_recovers_after_postgres_bounce` | **guard (PER-15065)** | PASSES on this branch (which has #915); guards the in-place broadcaster reconnect | + +Notes on the two guards: +- `test_repeat_sync_rss_stays_bounded` — clone paths are keyed by the repo URL, + so re-syncing identical scopes reuses cache entries and the cache *counts* + can't grow for any implementation; the load-bearing assertion is therefore on + RSS only (a `len(repos)` check would be tautological and is intentionally + omitted), guarding against a regression that leaks per-sync allocations. +- `test_server_recovers_after_postgres_bounce` — runs **2 workers** so the + Postgres backbone is actually exercised (cross-worker fan-out needs >=2 + workers; a single worker fans out in-process and never touches the backbone). + Across a transient bounce it asserts the gunicorn **worker PIDs are unchanged** + — proving #915's reconnecting broadcaster recovered the reader *in place* + rather than gunicorn respawning a graceful-shutdown worker (the pre-fix + behaviour) — and that a scope PUT after the bounce becomes servable, proving + the broadcast/sync path recovered (not just HTTP). ## Requires Docker + docker compose v2, plus host Python with `pytest pytest-timeout requests GitPython`. diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index 446376e3f..37c8e2d37 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -80,6 +80,38 @@ def opal(stack) -> OpalServerClient: stack.delete_all_scopes() +@pytest.fixture() +def opal_multiworker(stack) -> OpalServerClient: + """opal_server reconfigured to 2 gunicorn workers, for the broadcaster + test. + + The session stack is single-worker (the right call for the per- + process cache drain assertions), but the Postgres broadcaster's + cross-worker fan-out — the reason it is in this compose file at all + — is only exercised with >=2 workers (references/debug-pubsub.md + §3-4). This force-recreates opal_server with 2 workers for one test, + then restores the single-worker stack on teardown so the cache tests + keep their determinism. Each side starts from a clean slate: the + recreate wipes the container's on-disk clones, and clearing scopes + stops a leftover scope (whose clone is URL-keyed) from being re- + cloned on boot. + """ + os.environ["OPAL_TEST_WORKERS"] = "2" + try: + # --no-deps: don't bounce redis/postgres/gitea; --force-recreate: apply + # the new worker count. No --wait (opal_server has no compose + # healthcheck) — wait_healthy() polls the HTTP surface instead. + compose("up", "-d", "--no-deps", "--force-recreate", "opal_server") + stack.wait_healthy() + stack.delete_all_scopes() + yield stack + finally: + os.environ["OPAL_TEST_WORKERS"] = "1" + compose("up", "-d", "--no-deps", "--force-recreate", "opal_server") + stack.wait_healthy() + stack.delete_all_scopes() + + @pytest.fixture(scope="session") def gitea_admin(stack) -> GiteaAdmin: """Host-side Gitea admin client (depends on `stack` so Gitea is up).""" diff --git a/app-tests/git-leak/docker-compose.yml b/app-tests/git-leak/docker-compose.yml index a863faed4..4e709264b 100644 --- a/app-tests/git-leak/docker-compose.yml +++ b/app-tests/git-leak/docker-compose.yml @@ -97,15 +97,25 @@ services: dockerfile: docker/Dockerfile target: server environment: - # Single worker on purpose: the GitPolicyFetcher caches read by + # Default single worker: the GitPolicyFetcher caches read by # /internal/git-fetcher-cache-stats are per-process, so with >1 worker a # round-robin read can miss the worker that fetched and make a `== 0` # drain assertion pass falsely. One worker makes every cache read # deterministic. The leak/boot/offline bugs all reproduce single-worker. - UVICORN_NUM_WORKERS: "1" + # The postgres-bounce test (test_resilience.py) overrides this to 2 via + # OPAL_TEST_WORKERS for its own container, because cross-worker fan-out + # over the Postgres backbone only happens with >=2 workers + # (references/debug-pubsub.md §3-4) — a single worker can't tell #915's + # in-place broadcaster reconnect from a plain worker respawn. + UVICORN_NUM_WORKERS: "${OPAL_TEST_WORKERS:-1}" OPAL_SCOPES: "1" OPAL_REDIS_URL: "redis://redis:6379" OPAL_BROADCAST_URI: "postgres://opal:opal@postgres:5432/opal" + # Make the broadcaster reconnect fast + deterministic for the postgres- + # bounce test (defaults are 30s max backoff / 2s settle). Harmless to the + # single-worker tests, which don't exercise cross-worker fan-out. + OPAL_BROADCAST_RECONNECT_BACKOFF_MAX_SECONDS: "2" + OPAL_BROADCAST_RESYNC_SETTLE_SECONDS: "2" OPAL_BASE_DIR: "/opal" OPAL_POLICY_REFRESH_INTERVAL: "0" OPAL_DEBUG_INTERNAL_STATS: "1" diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 452d16b99..7a246ce63 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -285,6 +285,50 @@ def compose(*args: str) -> subprocess.CompletedProcess: return proc +def worker_pids(service: str = "opal_server") -> set: + """Return the set of gunicorn *worker* PIDs running inside ``service``. + + The server runs ``gunicorn`` (master) + ``UvicornWorker`` children (see + ``scripts/start.sh``). When a worker's broadcaster reader gives up on a + backbone disconnect it triggers a graceful shutdown and gunicorn respawns + the worker with a *new* PID; the reconnecting broadcaster (PER-15065 / #915) + instead recovers the reader in place and the worker keeps its PID. Comparing + this set across a transient bounce is how the broadcaster test tells an + in-place reconnect apart from a worker respawn. + + Implemented over ``/proc`` (no ``ps`` in the slim image): every gunicorn + process' ``cmdline`` contains "gunicorn", and the master is the lowest PID + (it exists before it forks any worker), so the workers are the rest. The + match is done **host-side in Python**, not with ``grep gunicorn`` in the + container: the scanning command's own ``sh -c`` wrapper has "gunicorn" in + its command line, so an in-container grep would count that wrapper as a + third "worker". The dump command below contains neither "gunicorn" nor + "grep", so it cannot match itself. + """ + out = compose( + "exec", + "-T", + service, + "sh", + "-c", + # emit " " per process; tr -d strips the NUL arg + # separators so the args concatenate into one searchable token. + # `|| true`: a momentary read failure must not raise from compose(). + "for d in /proc/[0-9]*/; do p=${d#/proc/}; p=${p%/}; " + 'echo "$p $(cat "$d/cmdline" 2>/dev/null | tr -d "\\000")"; ' + "done || true", + ).stdout + pids = [] + for line in out.splitlines(): + pid_str, _, cmd = line.partition(" ") + if pid_str.isdigit() and "gunicorn" in cmd: + pids.append(int(pid_str)) + pids.sort() + if len(pids) <= 1: + return set() # only the master (or nothing) observed: no workers + return set(pids[1:]) # drop the master (lowest PID); the rest are workers + + def bounce_postgres(down_seconds: int = 5) -> None: compose("stop", "postgres") time.sleep(down_seconds) diff --git a/app-tests/git-leak/test_boot.py b/app-tests/git-leak/test_boot.py index b28637ec1..28e57852b 100644 --- a/app-tests/git-leak/test_boot.py +++ b/app-tests/git-leak/test_boot.py @@ -2,41 +2,66 @@ import time import pytest +import requests from helpers import compose, gitea_repo_url, list_seeded_repos @pytest.mark.timeout(2400) def test_boot_loads_all_scopes(opal, repo_count): - """Measure how long a fresh boot takes to load all scope repos. + """Measure how long a fresh boot takes to *serve* all scope repos. - On master this is serial and slow (the ~20-min problem at scale). - PR4 tightens BOOT_TARGET_SECONDS to assert the parallel speedup. + On master boot-sync is serial and slow (the ~20-min problem at scale). PR1 + records the baseline (loose target, so it passes here as a baseline + recorder); PR4 (boot parallelism) is what makes this a hard gate. + + Carry-forward for PR4: run with a tight ``BOOT_TARGET_SECONDS`` (the plan: + 120s @ 50 scopes) so the assertion actually gates the parallel-boot fix — + with the default loose target it always passes and only records a number. + + Completion is keyed on every scope's bundle being *served* + (``GET /scopes/{id}/policy`` == 200), not on the ``repo_locks`` cache count: + ``repo_locks`` is set at fetch *start*, so stopping the clock on it would end + before the final clone finishes and undercount the boot-sync window this gate + exists to measure. """ n = repo_count repos = list_seeded_repos(n) - for i, name in enumerate(repos): - opal.put_scope(f"boot-{i}", gitea_repo_url(name)) + scope_ids = [f"boot-{i}" for i in range(n)] + for scope_id, name in zip(scope_ids, repos): + opal.put_scope(scope_id, gitea_repo_url(name)) # Start the clock at the restart, not after wait_healthy: preload_scopes() # runs in gunicorn's `when_ready` (before workers accept traffic), so by the # time /healthcheck answers, boot-sync may already be partly done — starting - # the clock later would undercount it. Measuring from the restart captures - # the whole boot-sync window. + # the clock later would undercount it. (Clones survive a restart, so preload + # re-discovers the on-disk repos rather than re-cloning from scratch.) start = time.time() compose("restart", "opal_server") opal.wait_healthy(timeout=600) - deadline = start + 2000 - loaded = 0 - while time.time() < deadline: - loaded = opal.stats()["repo_locks"] - if loaded >= n: + # Poll until every scope serves its bundle. Re-check only the not-yet-served + # ones so the work drains as scopes come online; a not-yet-cloned scope + # returns a non-200 quickly (it does not block), so this stays cheap. + served = set() + poll_deadline = time.time() + 1800 + while time.time() < poll_deadline: + for scope_id in scope_ids: + if scope_id in served: + continue + try: + if opal.get_scope_policy(scope_id).status_code == 200: + served.add(scope_id) + except requests.RequestException: + pass + if len(served) == n: break time.sleep(2) elapsed = time.time() - start - # PR1 records the baseline (loose). PR4 will set BOOT_TARGET_SECONDS low. BOOT_TARGET_SECONDS = int(os.environ.get("BOOT_TARGET_SECONDS", "2000")) - print(f"boot loaded {n} scopes in {elapsed:.1f}s (target {BOOT_TARGET_SECONDS}s)") - assert loaded >= n, "not all scopes loaded after boot" + print( + f"boot served {len(served)}/{n} scopes in {elapsed:.1f}s " + f"(target {BOOT_TARGET_SECONDS}s)" + ) + assert len(served) == n, f"only {len(served)}/{n} scopes served after boot" assert elapsed < BOOT_TARGET_SECONDS, f"boot too slow: {elapsed:.1f}s" diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 09fe9034d..b2c208d0f 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -78,23 +78,24 @@ def _all_caches_empty() -> bool: @pytest.mark.timeout(900) -def test_repeat_sync_does_not_grow(opal, repo_count): - """Re-syncing the *same* scopes must not grow memory unboundedly. - - Idempotency guard, not a known-broken case — PASSES on this branch. Clone - paths are keyed by the repo URL (``source_id`` = sha256(url)+branch-shard), - so re-syncing identical scopes reuses the existing ``repos`` / - ``repos_last_fetched`` entries rather than allocating new ones; the cache - *counts* therefore can't grow for any implementation, which is why the - load-bearing assertion here is on **RSS**, not on ``len(repos)``. It guards - against a regression where each repeat sync leaks per-sync allocations. The - unbounded growth-then-no-purge-on-delete leak is covered by - ``test_churn_releases_caches`` above, which uses distinct scopes. +def test_repeat_sync_rss_stays_bounded(opal, repo_count): + """Re-syncing the *same* scopes must not leak per-sync memory (RSS guard). + + Deliberately an **RSS** guard, not a cache-count leak gate. The clone caches + are keyed by repo URL (``source_id`` = sha256(url)+branch-shard), so + re-syncing identical scopes reuses the existing ``repos`` / + ``repos_last_fetched`` entries; the cache *counts* cannot grow for *any* + implementation. A ``len(repos)`` assertion would therefore be tautological + (it can't fail), so it is intentionally omitted here — do not re-add it as a + "gate". The load-bearing assertion is RSS: it catches a regression where each + repeat sync leaks per-sync allocations even while the entry count stays flat. + + The unbounded-growth-then-no-purge-on-delete leak is covered by + ``test_churn_releases_caches`` above, which uses *distinct* scopes. """ n = min(repo_count, 50) repos = list_seeded_repos(n) loaded = _load_scopes(opal, "stable", repos) - baseline_repos = loaded["repos"] baseline_rss = loaded["rss_kb"] for _ in range(10): @@ -102,9 +103,6 @@ def test_repeat_sync_does_not_grow(opal, repo_count): time.sleep(2) grown = opal.stats() - assert ( - grown["repos"] <= baseline_repos - ), f"repos cache count grew on repeat sync: {baseline_repos} -> {grown['repos']}" # allow generous headroom for allocator slack; fail only on a real per-sync # leak (10 refreshes of N scopes ballooning RSS). rss_budget = baseline_rss + max(50_000, baseline_rss // 5) diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index 04702448b..cf0e4914b 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -8,6 +8,7 @@ gitea_repo_url, list_seeded_repos, make_repo_unreachable, + worker_pids, ) # Enough hanging clones to exhaust opal's default fetch executor @@ -80,33 +81,41 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): opal.hard_reset() -@pytest.mark.timeout(300) -def test_server_recovers_after_postgres_bounce(opal, repo_count): - """A transient Postgres (broadcaster) outage must not break propagation. +@pytest.mark.timeout(420) +def test_server_recovers_after_postgres_bounce(opal_multiworker, repo_count): + """A transient Postgres (broadcaster) outage must reconnect *in place*. + + Runs **2 workers** (via the ``opal_multiworker`` fixture) so the Postgres + backbone is actually exercised: cross-worker fan-out only happens with >=2 + workers (references/debug-pubsub.md §3-4). A single worker fans out + in-process and never touches the backbone, so it can't tell #915's in-place + reconnect from a plain worker respawn — which is why the previous + single-worker version of this test passed either way. - Recovery guard, not a known-broken case — PASSES on this branch: when the - broadcast channel drops, the affected worker triggers a graceful shutdown - and gunicorn respawns it, and once Postgres is back the broadcaster - reconnects. We prove recovery of the *broadcast path*, not just HTTP - liveness: after the bounce we PUT a fresh scope and assert it actually - syncs (its repo lands in the cache), which only happens if the leader - received the sync notification over the recovered broadcaster. - (PER-15065's in-process reconnect would make recovery cleaner by avoiding - the worker churn, but recovery already holds.) + Guards PER-15065 (#915): on a backbone disconnect the reconnecting + broadcaster recovers the reader in process (retry-forever by default), so the + worker keeps its PID. Before that fix the disconnect escalated to a graceful + worker shutdown and gunicorn respawned the worker with a *new* PID. We assert + (a) the worker PIDs are unchanged across the bounce — the in-place-reconnect + signal — and (b) a scope PUT after the bounce becomes servable, proving the + broadcast/sync path itself recovered (not just HTTP liveness). """ - baseline = opal.stats() # healthy before - assert baseline - baseline_locks = baseline["repo_locks"] + opal = opal_multiworker + + before = worker_pids() + assert ( + len(before) == 2 + ), f"expected 2 gunicorn workers for this test, got {sorted(before)}" bounce_postgres(down_seconds=5) - # wait for the HTTP surface to come back first + # HTTP must come back first. A respawn would also satisfy this — hence the + # PID check below, which a respawn would *not* satisfy. deadline = time.time() + 90 recovered = False while time.time() < deadline: try: opal.wait_healthy(timeout=5) - opal.stats() recovered = True break except (requests.RequestException, RuntimeError): @@ -114,16 +123,36 @@ def test_server_recovers_after_postgres_bounce(opal, repo_count): time.sleep(2) assert recovered, "server did not recover HTTP within 90s of a postgres bounce" - # prove the broadcast path itself recovered: a freshly PUT scope must sync + # (a) in-place reconnect: the workers must be the *same* processes. A changed + # PID means the broadcaster gave up and gunicorn respawned the worker — the + # pre-#915 behavior this guards against. + after = worker_pids() + assert after == before, ( + f"workers respawned across the bounce ({sorted(before)} -> {sorted(after)}); " + f"the broadcaster did not reconnect in place (PER-15065 regressed)" + ) + + # (b) the broadcast/sync path recovered: a freshly PUT scope must become + # servable. A 200 from its bundle proves the leader received the sync and + # cloned the repo after the backbone returned. We assert on a served bundle + # rather than /internal cache counts, which are per-process and so not + # deterministic to read on a 2-worker stack. healthy = list_seeded_repos(1)[0] opal.put_scope("post-bounce", gitea_repo_url(healthy)) - synced = False + served = False + last = None deadline = time.time() + 120 while time.time() < deadline: - if opal.stats()["repo_locks"] > baseline_locks: - synced = True - break + try: + resp = opal.get_scope_policy("post-bounce") + last = resp.status_code + if resp.status_code == 200: + served = True + break + except requests.RequestException as exc: + last = repr(exc) time.sleep(2) - assert ( - synced - ), "scope PUT after the bounce never synced; broadcaster did not recover" + assert served, ( + f"scope PUT after the bounce never became servable (last: {last}); " + f"the broadcaster/sync path did not recover" + ) From 6f002fda3cece2f8b160d2ecc15c678765c095dd Mon Sep 17 00:00:00 2001 From: David Shoen Date: Wed, 1 Jul 2026 13:02:50 +0300 Subject: [PATCH 15/17] test(git-leak): address PR review round 3 (fixture robustness + cleanup) Zeev's latest inline batch: - pytest.ini: self-root the suite (app-tests/git-leak/pytest.ini) so `cd app-tests/git-leak && pytest --boot-scopes=N` is deterministic across pytest versions/cwd. (The documented command already works -- testpaths only applies when pytest runs from the rootdir -- but this makes it explicit.) - compose(): add a subprocess timeout (default 1200s) so a wedged up/wait/build fails fast instead of hanging session-scoped fixture setup to the CI job limit (pytest-timeout does not cover fixture setup). - delete_all_scopes(): cut the drain wait 20s -> 3s; on master the caches can't purge (the leak this gates), so the old wait burned ~40s of dead time per test across setup+teardown. - seed_gitea.py: inject push creds scheme-agnostically (urllib.parse) instead of string-replacing "http://"; drop the unused /seed-output token artifact and the seed-output volume (host uses basic auth, never the token). The order-dependent bounce signal (test_resilience.py) was already fixed in 8e24cb0b (asserts GET /scopes/post-bounce/policy == 200, not a delta on a shared process-global counter). Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/docker-compose.yml | 3 --- app-tests/git-leak/helpers.py | 39 +++++++++++++++++++-------- app-tests/git-leak/pytest.ini | 11 ++++++++ app-tests/git-leak/seed/seed_gitea.py | 11 +++++--- 4 files changed, 46 insertions(+), 18 deletions(-) create mode 100644 app-tests/git-leak/pytest.ini diff --git a/app-tests/git-leak/docker-compose.yml b/app-tests/git-leak/docker-compose.yml index 4e709264b..dde5a892c 100644 --- a/app-tests/git-leak/docker-compose.yml +++ b/app-tests/git-leak/docker-compose.yml @@ -87,8 +87,6 @@ services: GITEA_ADMIN_USER: "opaladmin" GITEA_ADMIN_PASSWORD: "opaladmin" REPO_COUNT: "${REPO_COUNT:-50}" - volumes: - - seed-output:/seed-output restart: "no" opal_server: @@ -133,4 +131,3 @@ services: volumes: gitea-data: - seed-output: diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index 7a246ce63..c455b3538 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -126,15 +126,20 @@ def hard_reset(self, timeout: int = 600) -> None: self._created_scopes.clear() self.wait_healthy(timeout=timeout) - def delete_all_scopes(self, drain_timeout: int = 20) -> None: + def delete_all_scopes(self, drain_timeout: int = 3) -> None: """Delete every scope the *server* knows (not just this client's), then best-effort wait for the caches to drain — a clean slate independent of what any prior, possibly-failed, test left behind. Best-effort drain by design: on master, delete never purges the caches - (the leak this suite gates), so the wait simply times out. This runs in - fixture setup/teardown, so a failure here must not mask the test, hence - the broad excepts and bounded wait. + (the leak this suite gates), so the wait can't succeed there — hence the + short ``drain_timeout`` (this runs in *every* test's setup and teardown, + so a long wait for a state that can't occur on master would be pure dead + time per test). Post-PR2 the purge is near-instant, so a few seconds is + ample. The DELETEs themselves are synchronous, so the scope store is + already clean before this wait — the wait only smooths the in-process + cache count. This runs in fixture setup/teardown, so a failure here must + not mask the test, hence the broad excepts and bounded wait. """ try: for scope_id in self.list_scope_ids(): @@ -263,20 +268,32 @@ def make_repo_unreachable(name: str) -> str: return f"http://{UNREACHABLE_HOST}/{GITEA_USER}/{name}.git" -def compose(*args: str) -> subprocess.CompletedProcess: +def compose(*args: str, timeout: int = 1200) -> subprocess.CompletedProcess: """Run `docker compose `; on failure, surface the captured output. `capture_output=True` keeps compose noise out of passing tests, but a raw CalledProcessError shows only the exit code — so on failure we re-raise with the captured stdout/stderr embedded, otherwise a broken build/seed/ restart is opaque to debug. + + ``timeout`` (default 1200s) bounds each call: ``@pytest.mark.timeout`` does + not cover session-scoped *fixture setup*, so a wedged ``up``/``wait``/build + would otherwise hang to the CI job limit. On expiry we raise a clear error + (subprocess.run kills the process group) instead of blocking indefinitely. """ - proc = subprocess.run( - ["docker", "compose", *args], - cwd=_COMPOSE_DIR, - capture_output=True, - text=True, - ) + try: + proc = subprocess.run( + ["docker", "compose", *args], + cwd=_COMPOSE_DIR, + capture_output=True, + text=True, + timeout=timeout, + ) + except subprocess.TimeoutExpired as exc: + raise RuntimeError( + f"`docker compose {' '.join(args)}` timed out after {timeout}s\n" + f"--- stdout ---\n{exc.stdout or ''}\n--- stderr ---\n{exc.stderr or ''}" + ) from exc if proc.returncode != 0: raise RuntimeError( f"`docker compose {' '.join(args)}` failed (exit {proc.returncode})\n" diff --git a/app-tests/git-leak/pytest.ini b/app-tests/git-leak/pytest.ini new file mode 100644 index 000000000..0d501534f --- /dev/null +++ b/app-tests/git-leak/pytest.ini @@ -0,0 +1,11 @@ +[pytest] +# Self-root the git-leak suite so `cd app-tests/git-leak && pytest --boot-scopes=N` +# is deterministic regardless of pytest version or cwd. +# +# The repo-root pytest.ini sets `testpaths = packages` to keep this docker suite +# out of the CI unit run. Per pytest, `testpaths` only applies when pytest is +# invoked *from the rootdir*, so running from here already collects this dir and +# loads its conftest (where --boot-scopes is registered). This empty [pytest] +# marker makes that guarantee explicit — it pins the rootdir to this directory +# when run in place, independent of the root config — while the root run (from +# the repo root) still never descends here. diff --git a/app-tests/git-leak/seed/seed_gitea.py b/app-tests/git-leak/seed/seed_gitea.py index 7e1de1a4c..674a54689 100644 --- a/app-tests/git-leak/seed/seed_gitea.py +++ b/app-tests/git-leak/seed/seed_gitea.py @@ -13,6 +13,7 @@ import sys import time from pathlib import Path +from urllib.parse import urlsplit, urlunsplit import requests from git import Actor, Repo @@ -104,8 +105,12 @@ def _push_policy( author = Actor("seed", "seed@example.com") repo.index.commit("seed policy", author=author, committer=author) - push_url = ( - base_url.replace("http://", f"http://{user}:{token}@") + f"/{user}/{name}.git" + # Inject credentials scheme-agnostically (works for http and https) rather + # than string-replacing "http://", which would silently drop the creds if + # GITEA_URL were ever https and produce an opaque auth failure. + parts = urlsplit(base_url) + push_url = urlunsplit( + (parts.scheme, f"{user}:{token}@{parts.netloc}", f"/{user}/{name}.git", "", "") ) origin = repo.create_remote("origin", push_url) origin.push(refspec="main:main") @@ -155,8 +160,6 @@ def main() -> int: ) return 1 - # write the token where the test harness can read it - Path("/seed-output/token").write_text(token) print(f"DONE: ensured {total} repos", flush=True) return 0 From 5aae85ca9f32dd3294e652cfae2adaf71cd76b50 Mon Sep 17 00:00:00 2001 From: David Shoen Date: Wed, 1 Jul 2026 15:15:13 +0300 Subject: [PATCH 16/17] test(git-leak): harden gates per multi-agent review (H1 + M1-M6) Addresses the HIGH and all MEDIUM findings from the opal-development / python-pro / backend-architect microreview: - H1 (#5 vacuous pass): the broadcaster gate now positively verifies a disconnect+reconnect actually happened, not just that no respawn occurred. New helpers.broadcaster_connect_count() counts the reconnecting broadcaster's "listener connected to channel" log line; the test asserts it increased across the bounce (paired with worker PIDs unchanged = in-place, not respawn). - M1 (#4): move the 40 executor-saturating PUTs inside the try/finally so a PUT failure still runs hard_reset() instead of leaking hung clone threads into the session stack. - M2 (#1/#2): _wait_until now treats a transient requests error from opal.stats() as "not yet" and retries, instead of ERRORing the test. - M3 (#3): measure a deterministic pure-cold boot (--force-recreate wipes the ephemeral FS -> preload cold-clones all N from Redis) instead of a nondeterministic warm/cold mix, so PR4's tight BOOT_TARGET_SECONDS can gate. - M4: verify the single-worker invariant -- opal_multiworker teardown asserts the stack is back to 1 worker, and the opal fixture asserts single-worker at setup, so a botched restore fails loudly instead of silently breaking cache gates. - M5 (#4): correct the reserved-probe comment (serving shares the fetch executor, so a shared repo would be starved on serve too; the probe additionally exercises the clone). - M6: gitea-admin retries on "database is locked" (CLI mutating live SQLite); rewritten as a `|` literal block so the create call stays on one line. Validated live (--boot-scopes=20): #2/#3 pass, #5 passes (reconnect count increased across the bounce + PIDs unchanged + scope served), #1/#4 fail for the right reason. Co-Authored-By: Claude Opus 4.8 (1M context) --- app-tests/git-leak/conftest.py | 15 ++++++ app-tests/git-leak/docker-compose.yml | 30 ++++++----- app-tests/git-leak/helpers.py | 42 ++++++++++++---- app-tests/git-leak/test_boot.py | 18 +++++-- app-tests/git-leak/test_leak.py | 17 ++++++- app-tests/git-leak/test_resilience.py | 71 +++++++++++++++++++-------- 6 files changed, 147 insertions(+), 46 deletions(-) diff --git a/app-tests/git-leak/conftest.py b/app-tests/git-leak/conftest.py index 37c8e2d37..547c22404 100644 --- a/app-tests/git-leak/conftest.py +++ b/app-tests/git-leak/conftest.py @@ -8,6 +8,7 @@ OpalServerClient, compose, list_seeded_repos, + worker_pids, ) @@ -76,6 +77,13 @@ def opal(stack) -> OpalServerClient: # tracked set) at setup, so a scope orphaned by a prior failed test can't # contaminate this one; then again on teardown. stack.delete_all_scopes() + # Guard the single-worker invariant the cache gates depend on: if a prior + # opal_multiworker teardown failed to restore 1 worker, the per-process cache + # reads would be nondeterministic here (a `== 0` drain could false-pass). + # Fail loudly and ordering-independently rather than silently mis-measure. + assert ( + len(worker_pids()) == 1 + ), f"expected a single-worker stack, found workers {sorted(worker_pids())}" yield stack stack.delete_all_scopes() @@ -110,6 +118,13 @@ def opal_multiworker(stack) -> OpalServerClient: compose("up", "-d", "--no-deps", "--force-recreate", "opal_server") stack.wait_healthy() stack.delete_all_scopes() + # Verify the restore actually reduced the stack back to one worker. If it + # did not (a botched recreate), fail loudly here rather than leave a + # 2-worker stack that would silently break later single-worker cache + # gates' determinism. + assert ( + len(worker_pids()) == 1 + ), f"opal_multiworker teardown left workers {sorted(worker_pids())}, expected 1" @pytest.fixture(scope="session") diff --git a/app-tests/git-leak/docker-compose.yml b/app-tests/git-leak/docker-compose.yml index dde5a892c..66b5d7a59 100644 --- a/app-tests/git-leak/docker-compose.yml +++ b/app-tests/git-leak/docker-compose.yml @@ -51,18 +51,26 @@ services: condition: service_healthy user: git entrypoint: ["/bin/sh", "-c"] - # Tolerate only the idempotent "already exists" case; any other failure - # must abort so `seed` (which depends on this completing) doesn't run - # against a Gitea with no admin user and fail with a confusing 401. + # Tolerate the idempotent "already exists" case, and RETRY on "database is + # locked": this CLI mutates the same SQLite file the live gitea server holds + # open, so it can lose a lock race and fail transiently. Any other failure + # aborts so `seed` (which depends on this completing) doesn't run against a + # Gitea with no admin user and fail with a confusing 401. + # `|` (literal) block, not `>` (folded): the `gitea admin user create` call is + # kept on ONE line so YAML can't fold a newline into the middle of its args + # (that would run `--email ...` as its own command -> exit 127). command: - - > - out=$$(gitea admin user create --username opaladmin --password opaladmin - --email admin@example.com --admin --must-change-password=false - --config /data/gitea/conf/app.ini 2>&1); rc=$$?; - echo "$$out"; - if [ $$rc -ne 0 ] && ! echo "$$out" | grep -qi "already exist"; then - exit $$rc; - fi + - | + for attempt in 1 2 3 4 5 6; do + out=$$(gitea admin user create --username opaladmin --password opaladmin --email admin@example.com --admin --must-change-password=false --config /data/gitea/conf/app.ini 2>&1) + rc=$$? + echo "$$out" + if [ $$rc -eq 0 ] || echo "$$out" | grep -qi "already exist"; then exit 0; fi + if echo "$$out" | grep -qi "database is locked"; then echo "gitea db locked; retry $$attempt"; sleep 2; continue; fi + exit $$rc + done + echo "gitea admin create failed after retries" + exit 1 volumes: - gitea-data:/data restart: "no" diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index c455b3538..fd88c0d5a 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -346,6 +346,30 @@ def worker_pids(service: str = "opal_server") -> set: return set(pids[1:]) # drop the master (lowest PID); the rest are workers +# The reconnecting broadcaster (PER-15065 / #915) logs this line every time its +# reader (re)connects to the backbone channel — once at boot, and once more on +# each reconnect after a backbone drop (pubsub_resilience.py `_reader_loop` -> +# `_ensure_connected`). Counting it across a Postgres bounce positively proves a +# disconnect+reconnect actually happened. +_BROADCASTER_CONNECT_LOG = "Broadcaster listener connected to channel" + + +def broadcaster_connect_count(service: str = "opal_server") -> int: + """Count broadcaster reader (re)connect log lines for ``service``. + + The postgres-bounce test asserts this COUNT *increased* across the bounce so + the gate positively confirms the backbone actually dropped and the reader + reconnected — without this, a bounce that failed to break the reader (a + future Postgres shutdown-signal change, connection pooling, etc.) would leave + the worker PIDs unchanged and pass the gate vacuously. Paired with + ``worker_pids()`` unchanged (which proves the recovery was *in place*, not a + respawn), the two together pin down the PER-15065 property. + """ + # --no-log-prefix strips the "service | " column so the marker matches cleanly. + out = compose("logs", "--no-log-prefix", service).stdout + return out.count(_BROADCASTER_CONNECT_LOG) + + def bounce_postgres(down_seconds: int = 5) -> None: compose("stop", "postgres") time.sleep(down_seconds) @@ -361,13 +385,13 @@ def list_seeded_repos(count: int) -> List[str]: # A reserved repo seeded *outside* the numeric ``policy-repo-NNNN`` range that # ``list_seeded_repos`` enumerates, so no boot/leak test ever clones it. The -# resilience offline-hang test uses it as its "healthy" probe: clones live at -# ``base_dir/`` keyed by URL-hash and survive ``compose -# restart/stop/start`` (opal_server mounts no volume at ``/opal``; only -# ``down -v`` wipes them), so pointing the probe at any shared seeded repo would -# let the healthy scope reuse an on-disk clone and serve 200 *without* touching -# the saturated fetch executor — false-passing a gate that must FAIL on this -# branch. A dedicated never-cloned repo forces a genuine fresh clone through the -# starved executor. Keep this name in sync with ``RESERVED_REPOS`` in -# ``seed/seed_gitea.py``. +# resilience offline-hang test uses it as its "healthy" probe so the scope must +# perform a genuine *clone* through the starved executor, rather than reusing an +# on-disk clone left by another test (clones live at ``base_dir/`` +# keyed by URL-hash and survive ``compose restart/stop/start`` — opal_server +# mounts no volume at ``/opal``; only ``down -v`` wipes them). Note serving the +# bundle (``make_bundle`` via ``run_sync``) shares that same fetch executor, so a +# pre-cloned shared repo would be starved on serve too — the never-cloned probe +# is belt-and-suspenders that additionally exercises the clone path. Keep this +# name in sync with ``RESERVED_REPOS`` in ``seed/seed_gitea.py``. HEALTHY_PROBE_REPO = "policy-repo-healthy-probe" diff --git a/app-tests/git-leak/test_boot.py b/app-tests/git-leak/test_boot.py index 28e57852b..f0b66c697 100644 --- a/app-tests/git-leak/test_boot.py +++ b/app-tests/git-leak/test_boot.py @@ -18,6 +18,14 @@ def test_boot_loads_all_scopes(opal, repo_count): 120s @ 50 scopes) so the assertion actually gates the parallel-boot fix — with the default loose target it always passes and only records a number. + We measure a **deterministic cold boot**: ``--force-recreate`` gives the + container a fresh filesystem (opal_server mounts no volume at ``/opal``), so + ``preload_scopes`` must cold-clone all N repos from the Redis scope store — + exactly the fresh-pod scenario PR4 parallelizes. A plain ``restart`` would + instead leave a run-dependent mix of complete on-disk clones (fast warm + re-discover) and clones killed mid-flight by the restart (cold re-clone), + making ``elapsed`` non-deterministic and a poor basis for PR4's tight gate. + Completion is keyed on every scope's bundle being *served* (``GET /scopes/{id}/policy`` == 200), not on the ``repo_locks`` cache count: ``repo_locks`` is set at fetch *start*, so stopping the clock on it would end @@ -30,13 +38,15 @@ def test_boot_loads_all_scopes(opal, repo_count): for scope_id, name in zip(scope_ids, repos): opal.put_scope(scope_id, gitea_repo_url(name)) - # Start the clock at the restart, not after wait_healthy: preload_scopes() + # Start the clock at the recreate, not after wait_healthy: preload_scopes() # runs in gunicorn's `when_ready` (before workers accept traffic), so by the # time /healthcheck answers, boot-sync may already be partly done — starting - # the clock later would undercount it. (Clones survive a restart, so preload - # re-discovers the on-disk repos rather than re-cloning from scratch.) + # the clock later would undercount it. --force-recreate wipes the container FS + # so the scopes (still in Redis) are all cold-cloned by preload, a + # deterministic fresh-boot measurement. --no-deps leaves redis/postgres/gitea + # untouched. start = time.time() - compose("restart", "opal_server") + compose("up", "-d", "--no-deps", "--force-recreate", "opal_server") opal.wait_healthy(timeout=600) # Poll until every scope serves its bundle. Re-check only the not-yet-served diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index b2c208d0f..800b23afc 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -1,14 +1,27 @@ import time import pytest +import requests from helpers import gitea_repo_url, list_seeded_repos def _wait_until(predicate, timeout=30, interval=0.5): + """Poll ``predicate`` until true or ``timeout`` elapses. + + The predicates here read ``/internal`` via ``opal.stats()``, which does + ``raise_for_status()``. A *transient* request error (a momentary 5xx, a + connection reset, or a read racing an in-flight mutation) is treated as "not + yet" and retried instead of propagating and turning the whole test into an + ERROR before the deadline. A *persistent* failure still surfaces: the wait + just returns ``False`` and the caller's ``assert`` fires with the last stats. + """ deadline = time.time() + timeout while time.time() < deadline: - if predicate(): - return True + try: + if predicate(): + return True + except requests.RequestException: + pass time.sleep(interval) return False diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index cf0e4914b..d2ef2fd3e 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -5,6 +5,7 @@ from helpers import ( HEALTHY_PROBE_REPO, bounce_postgres, + broadcaster_connect_count, gitea_repo_url, list_seeded_repos, make_repo_unreachable, @@ -28,24 +29,29 @@ def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): fetch executor and the healthy scope's bundle never becomes available. """ - # the `blackhole` sidecar accepts the TCP handshake then never answers, so - # each of these clones hangs (holding a fetch-executor thread) rather than - # failing fast; enough of them saturate the pool. - for i in range(OFFLINE_REPOS): - opal.put_scope( - f"offline-{i}", make_repo_unreachable(f"dead-{i}"), branch="main" - ) - - # Point the healthy scope at a repo *no other test clones* (HEALTHY_PROBE_REPO - # is seeded outside the numeric range the boot/leak tests enumerate). Clones - # survive compose restart/stop/start, so reusing a shared seeded repo here - # would let this scope hit the existing on-disk clone via _discover_repository, - # skip _clone(), and serve 200 without ever touching the saturated executor — - # false-passing this gate. A never-cloned repo forces a real clone through the - # starved pool, so the gate fails correctly on this branch (no PR3 timeout). - opal.put_scope("healthy", gitea_repo_url(HEALTHY_PROBE_REPO)) - try: + # the `blackhole` sidecar accepts the TCP handshake then never answers, so + # each of these clones hangs (holding a fetch-executor thread) rather than + # failing fast; enough of them saturate the pool. These PUTs live *inside* + # the try so that if one fails partway through, the finally still runs + # hard_reset() — otherwise the clones already dispatched to the executor + # would hang for the blackhole's full duration and starve the + # session-scoped stack for every later test. + for i in range(OFFLINE_REPOS): + opal.put_scope( + f"offline-{i}", make_repo_unreachable(f"dead-{i}"), branch="main" + ) + + # Point the healthy scope at a repo *no other test clones* + # (HEALTHY_PROBE_REPO is seeded outside the numeric range the boot/leak + # tests enumerate) so it must perform a genuine clone rather than reuse a + # surviving on-disk clone via _discover_repository. Serving the bundle + # shares the same starved executor too, so a shared repo would also stay + # red here — the never-cloned probe additionally guarantees the *clone* + # itself is exercised through the starved pool (fails correctly on this + # branch, no PR3 timeout). + opal.put_scope("healthy", gitea_repo_url(HEALTHY_PROBE_REPO)) + # The healthy scope must become *servable* within a bounded time even # while the offline scopes hang. A 200 from its policy bundle proves the # clone completed and the scope is served — a stronger signal than a @@ -97,8 +103,10 @@ def test_server_recovers_after_postgres_bounce(opal_multiworker, repo_count): worker keeps its PID. Before that fix the disconnect escalated to a graceful worker shutdown and gunicorn respawned the worker with a *new* PID. We assert (a) the worker PIDs are unchanged across the bounce — the in-place-reconnect - signal — and (b) a scope PUT after the bounce becomes servable, proving the - broadcast/sync path itself recovered (not just HTTP liveness). + signal; (b) the backbone reader actually dropped and reconnected (its connect + log count increased), so (a) is not vacuously true because the bounce failed + to break anything; and (c) a scope PUT after the bounce becomes servable, + proving the broadcast/sync path itself recovered (not just HTTP liveness). """ opal = opal_multiworker @@ -106,6 +114,8 @@ def test_server_recovers_after_postgres_bounce(opal_multiworker, repo_count): assert ( len(before) == 2 ), f"expected 2 gunicorn workers for this test, got {sorted(before)}" + # baseline reader (re)connect count — assertion (b) requires it to increase + before_connects = broadcaster_connect_count() bounce_postgres(down_seconds=5) @@ -132,7 +142,28 @@ def test_server_recovers_after_postgres_bounce(opal_multiworker, repo_count): f"the broadcaster did not reconnect in place (PER-15065 regressed)" ) - # (b) the broadcast/sync path recovered: a freshly PUT scope must become + # (b) the bounce actually broke and reconnected the backbone reader — so (a) + # is not vacuously true because nothing dropped. The reconnecting broadcaster + # logs a fresh connect line on every reconnect; the count must strictly + # increase over the pre-bounce baseline (poll, since the reconnect lags the + # Postgres healthcheck by the backoff + resync-settle window). + deadline = time.time() + 60 + reconnected = False + after_connects = before_connects + while time.time() < deadline: + after_connects = broadcaster_connect_count() + if after_connects > before_connects: + reconnected = True + break + time.sleep(2) + assert reconnected, ( + f"broadcaster reader never logged a reconnect after the bounce " + f"(connect count stayed {before_connects}); the bounce may not have " + f"dropped the backbone, which would make the PID-unchanged check above " + f"vacuous" + ) + + # (c) the broadcast/sync path recovered: a freshly PUT scope must become # servable. A 200 from its bundle proves the leader received the sync and # cloned the repo after the backbone returned. We assert on a served bundle # rather than /internal cache counts, which are per-process and so not From 9b61027975ad52ab709dd4368f37784fe4232009 Mon Sep 17 00:00:00 2001 From: zivxx Date: Sun, 12 Jul 2026 15:07:56 +0300 Subject: [PATCH 17/17] test(git-leak): add PR2 over-purge/update-path gates + during-outage publish guard (review items 1-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverage additions from the test-coverage review pass: 1. Unit tests for the server.py wiring: build the real OpalServer app and assert /internal/git-fetcher-cache-stats is mounted iff DEBUG_INTERNAL_STATS is on — removing the register call from _init_fast_api_app now fails the unit suite. 2. test_shared_repo_survives_sibling_scope_delete: deleting one of two scopes sharing a repo URL must not purge the URL-keyed cache entry or break the survivor (guards PR2 against over-purging; churn only covers the all-scopes-gone direction). 3. test_scope_repoint_releases_old_repo_cache: PUT /scopes is create-or-update, so re-pointing a scope orphans the old URL's cache entries — a leak path churn (delete-only) never takes. Red until PR2's purge covers updates too. 4. Postgres-bounce test: bounce_postgres gained a during= callback (with a finally that restores Postgres even if it raises) and the test now PUTs a scope mid-outage, asserting (d) it becomes servable after recovery — its sync trigger must be buffered/replayed across the gap, not silently dropped. Uses a distinct seeded repo from (c) so a stale on-disk clone can't satisfy it vacuously. Validated live (--boot-scopes=2): #2 and the bounce test pass; #3 fails for the right reason (caches stuck at 2/2/2 after the delete — PR2 not landed). Unit suite: 52 passed; pre-commit clean. Co-Authored-By: Claude Fable 5 --- app-tests/git-leak/README.md | 24 +++- app-tests/git-leak/helpers.py | 29 ++++- app-tests/git-leak/test_leak.py | 106 ++++++++++++++++++ app-tests/git-leak/test_resilience.py | 77 ++++++++++--- .../tests/debug_stats_endpoint_test.py | 46 ++++++++ 5 files changed, 252 insertions(+), 30 deletions(-) diff --git a/app-tests/git-leak/README.md b/app-tests/git-leak/README.md index 4b5d9c486..a569975db 100644 --- a/app-tests/git-leak/README.md +++ b/app-tests/git-leak/README.md @@ -24,8 +24,10 @@ Postgres and `blackhole` are internal to the compose network. `create_repo`, `delete_repo`); also exposed as the `gitea_admin` pytest fixture. - `make_repo_unreachable(name)` — git URL on the `blackhole` sidecar (completes the TCP handshake, never answers) so the clone hangs for the offline-repo test. -- `bounce_postgres(down_seconds)` — stop Postgres, then `up -d --wait` it back to - simulate a broadcaster outage and await readiness before the recovery poll. +- `bounce_postgres(down_seconds, during=None)` — stop Postgres, optionally run a + callback while it is down (the bounce test publishes a scope mid-outage), then + `up -d --wait` it back to simulate a broadcaster outage and await readiness + before the recovery poll. ## Run ```bash @@ -43,12 +45,14 @@ Gate-coverage matrix (what each flagship test actually does): | Test | Role | Behaviour here | |---|---|---| | `test_churn_releases_caches` | **gate (PR2)** | FAILS without the PR2 leak fix — delete leaves the caches populated; flips green when PR2 lands | +| `test_scope_repoint_releases_old_repo_cache` | **gate (PR2, update path)** | FAILS without PR2 — re-pointing a scope to a new URL orphans the old URL's cache entries; stays red after PR2 unless its purge also covers scope *updates*, not just deletes | +| `test_shared_repo_survives_sibling_scope_delete` | **over-purge guard (PR2)** | PASSES here (nothing purges on master); once PR2 lands it guards against purging a URL-keyed entry that a surviving sibling scope still references | | `test_offline_repo_does_not_block_healthy_scopes` | **gate (PR3)** | FAILS without the PR3 fetch timeout — 40 hung clones starve the executor so a healthy scope never serves; flips green when PR3 lands | | `test_boot_loads_all_scopes` | **baseline → gate (PR4)** | PASSES with the loose default target; set `BOOT_TARGET_SECONDS` low (plan: 120 @ 50) on PR4 to gate the parallel-boot fix | | `test_repeat_sync_rss_stays_bounded` | **RSS guard** | PASSES; an RSS-budget guard against per-sync allocation leaks (the cache *count* can't grow for any impl, so there is no count assertion — see below) | -| `test_server_recovers_after_postgres_bounce` | **guard (PER-15065)** | PASSES on this branch (which has #915); guards the in-place broadcaster reconnect | +| `test_server_recovers_after_postgres_bounce` | **guard (PER-15065 + gap publishes)** | PASSES on this branch (which has #915); guards the in-place broadcaster reconnect and that a scope PUT *during* the outage is buffered/replayed, not dropped | -Notes on the two guards: +Notes on the guards: - `test_repeat_sync_rss_stays_bounded` — clone paths are keyed by the repo URL, so re-syncing identical scopes reuses cache entries and the cache *counts* can't grow for any implementation; the load-bearing assertion is therefore on @@ -60,8 +64,16 @@ Notes on the two guards: Across a transient bounce it asserts the gunicorn **worker PIDs are unchanged** — proving #915's reconnecting broadcaster recovered the reader *in place* rather than gunicorn respawning a graceful-shutdown worker (the pre-fix - behaviour) — and that a scope PUT after the bounce becomes servable, proving - the broadcast/sync path recovered (not just HTTP). + behaviour) — that a scope PUT after the bounce becomes servable, proving + the broadcast/sync path recovered (not just HTTP), and that a scope PUT + *during* the outage becomes servable too: its sync trigger rides the + git-webhook topic, which the reconnecting broadcaster buffers and replays on + reconnect (and which #933's publish freeze exempts on master), so a 201 + acknowledged mid-gap must never be silently dropped. +- `test_shared_repo_survives_sibling_scope_delete` — the caches are keyed by + repo URL, not scope id, so it green-guards PR2 against purging an entry that + another live scope still references (churn only covers the all-scopes-gone + direction). ## Requires Docker + docker compose v2, plus host Python with `pytest pytest-timeout requests GitPython`. diff --git a/app-tests/git-leak/helpers.py b/app-tests/git-leak/helpers.py index fd88c0d5a..64a32abbe 100644 --- a/app-tests/git-leak/helpers.py +++ b/app-tests/git-leak/helpers.py @@ -370,13 +370,30 @@ def broadcaster_connect_count(service: str = "opal_server") -> int: return out.count(_BROADCASTER_CONNECT_LOG) -def bounce_postgres(down_seconds: int = 5) -> None: +def bounce_postgres(down_seconds: int = 5, during=None) -> None: + """Stop Postgres, optionally run ``during()`` while it is down, restart it. + + ``during`` lets a test act inside the outage window (e.g. publish a scope + while the backbone is down). It runs right after the stop; the remainder of + ``down_seconds`` is then slept so the outage lasts at least that long + regardless of how long the callback took. Postgres is brought back even if + the callback raises — otherwise one failed callback would leave the + session-scoped stack without its broadcaster for every later test — and the + callback's exception then propagates. + """ compose("stop", "postgres") - time.sleep(down_seconds) - # `up -d --wait` blocks until Postgres passes its healthcheck again (plain - # `compose start` has no --wait), so a recovery poll that follows isn't - # racing an unready broadcaster. --no-recreate keeps the same container. - compose("up", "-d", "--wait", "--no-recreate", "postgres") + stopped_at = time.time() + try: + if during is not None: + during() + finally: + remaining = down_seconds - (time.time() - stopped_at) + if remaining > 0: + time.sleep(remaining) + # `up -d --wait` blocks until Postgres passes its healthcheck again (plain + # `compose start` has no --wait), so a recovery poll that follows isn't + # racing an unready broadcaster. --no-recreate keeps the same container. + compose("up", "-d", "--wait", "--no-recreate", "postgres") def list_seeded_repos(count: int) -> List[str]: diff --git a/app-tests/git-leak/test_leak.py b/app-tests/git-leak/test_leak.py index 800b23afc..971823cdd 100644 --- a/app-tests/git-leak/test_leak.py +++ b/app-tests/git-leak/test_leak.py @@ -123,3 +123,109 @@ def test_repeat_sync_rss_stays_bounded(opal, repo_count): f"RSS grew unboundedly on repeat sync: " f"{baseline_rss} -> {grown['rss_kb']} kb (budget {rss_budget})" ) + + +@pytest.mark.timeout(900) +def test_shared_repo_survives_sibling_scope_delete(opal, repo_count): + """Deleting one of two scopes that share a repo URL must not purge the + shared cache entry or break the surviving scope. + + Guards PR2 against *over*-purging: the GitPolicyFetcher caches are keyed by + repo URL (source_id), not scope_id, and sharing one policy repo across many + scopes is a normal setup. ``test_churn_releases_caches`` only asserts the + caches drain when *all* scopes referencing a URL are gone — so a purge that + fires whenever *any* scope pointing at the URL is deleted would pass that + gate while breaking every surviving sibling. This is the inverse assertion. + + PASSES on this branch (master never purges, so the entry trivially + survives); becomes load-bearing the moment PR2's purge lands. + """ + repo = list_seeded_repos(1)[0] + url = gitea_repo_url(repo) + opal.put_scope("shared-a", url) + opal.put_scope("shared-b", url) + + # both scopes share one URL, so all three caches hold exactly one entry + locked = _wait_until(lambda: opal.stats()["repo_locks"] >= 1, timeout=600) + assert locked, f"shared repo never locked: {opal.stats()}" + opal.refresh_all() + fetched = _wait_until(lambda: opal.stats()["repos"] >= 1, timeout=600) + assert fetched, f"refresh never populated the shared repo: {opal.stats()}" + + # sanity: the survivor serves *before* the sibling delete, so a failure + # after it can only be caused by the delete + served = _wait_until( + lambda: opal.get_scope_policy("shared-b").status_code == 200, timeout=120 + ) + assert served, "shared-b never served before the sibling delete" + + opal.delete_scope("shared-a") + + # Give an (incorrect) URL-keyed purge time to fire, then assert the shared + # entry survived. A fixed sleep — not a wait-for-condition — because the + # expected outcome is "nothing changes", which has no event to wait on. + time.sleep(5) + stats = opal.stats() + assert stats["repo_locks"] >= 1 and stats["repos"] >= 1, ( + f"deleting scope shared-a purged the cache entry still referenced by " + f"shared-b (over-purge): {stats}" + ) + resp = opal.get_scope_policy("shared-b") + assert resp.status_code == 200, ( + f"shared-b stopped serving after its sibling was deleted " + f"(status {resp.status_code}) — the shared clone was torn down" + ) + + +@pytest.mark.timeout(900) +def test_scope_repoint_releases_old_repo_cache(opal, repo_count): + """Re-pointing a scope to a new repo URL, then deleting it, must drain ALL + cache entries — including the orphaned old URL's. + + ``PUT /scopes`` is create-or-update, so PUT-ing an existing scope_id with a + *different* repo URL orphans the old URL's cache entries: the same leak as + churn, via a path ``test_churn_releases_caches`` (delete-only) never takes. + A purge hooked solely into scope deletion would look up the scope's + *current* URL and leave the old entry behind forever. + + FAILS on this branch (without PR2, nothing purges — the same right-reason + failure as churn); stays red after PR2 unless its purge also covers the + update path (or sweeps entries no live scope references). + """ + assert repo_count >= 2, "needs --boot-scopes >= 2" + repo_a, repo_b = list_seeded_repos(2) + + opal.put_scope("repoint", gitea_repo_url(repo_a)) + locked = _wait_until(lambda: opal.stats()["repo_locks"] >= 1, timeout=600) + assert locked, f"initial repo never locked: {opal.stats()}" + opal.refresh_all() + fetched = _wait_until(lambda: opal.stats()["repos"] >= 1, timeout=600) + assert fetched, f"refresh never populated the initial repo: {opal.stats()}" + + # Re-point the same scope_id at a different repo; repo_a's entries are now + # referenced by no scope. The sync triggered by the PUT clones repo_b + # (filling repo_locks), and the refresh fills its repos / + # repos_last_fetched — so both URLs are visible in the caches, proof the + # orphan exists before the delete (this guards the setup, not the leak). + opal.put_scope("repoint", gitea_repo_url(repo_b)) + locked = _wait_until(lambda: opal.stats()["repo_locks"] >= 2, timeout=600) + assert locked, f"re-pointed repo never locked: {opal.stats()}" + opal.refresh_all() + fetched = _wait_until(lambda: opal.stats()["repos"] >= 2, timeout=600) + assert fetched, ( + f"expected cache entries for both the old and new URL after the " + f"re-point: {opal.stats()}" + ) + + opal.delete_scope("repoint") + + def _all_caches_empty() -> bool: + s = opal.stats(samples=1) + return s["repo_locks"] == 0 and s["repos"] == 0 and s["repos_last_fetched"] == 0 + + released = _wait_until(_all_caches_empty, timeout=60) + assert released, ( + f"caches did not drain after deleting the re-pointed scope (the old " + f"URL's orphaned entry leaks unless the purge covers updates too): " + f"{opal.stats()}" + ) diff --git a/app-tests/git-leak/test_resilience.py b/app-tests/git-leak/test_resilience.py index d2ef2fd3e..d2df056f0 100644 --- a/app-tests/git-leak/test_resilience.py +++ b/app-tests/git-leak/test_resilience.py @@ -20,6 +20,26 @@ OFFLINE_REPOS = 40 +def _wait_served(opal, scope_id: str, timeout: int): + """Poll a scope's policy bundle until it serves 200 or ``timeout`` elapses. + + Returns ``(served, last)`` where ``last`` is the final status code or + exception repr, for the caller's assertion message. + """ + deadline = time.time() + timeout + last = None + while time.time() < deadline: + try: + resp = opal.get_scope_policy(scope_id) + last = resp.status_code + if resp.status_code == 200: + return True, last + except requests.RequestException as exc: + last = repr(exc) + time.sleep(2) + return False, last + + @pytest.mark.timeout(420) def test_offline_repo_does_not_block_healthy_scopes(opal, repo_count): """Unreachable repos must not stop a healthy scope from serving. @@ -105,10 +125,24 @@ def test_server_recovers_after_postgres_bounce(opal_multiworker, repo_count): (a) the worker PIDs are unchanged across the bounce — the in-place-reconnect signal; (b) the backbone reader actually dropped and reconnected (its connect log count increased), so (a) is not vacuously true because the bounce failed - to break anything; and (c) a scope PUT after the bounce becomes servable, - proving the broadcast/sync path itself recovered (not just HTTP liveness). + to break anything; (c) a scope PUT after the bounce becomes servable, + proving the broadcast/sync path itself recovered (not just HTTP liveness); + and (d) a scope PUT *during* the outage becomes servable too. + + (d) guards the disconnect-window publish path: the scope PUT publishes its + sync trigger on the git-webhook topic, which the reconnecting broadcaster + buffers while the backbone is down and replays on reconnect (and which + #933's publish freeze explicitly exempts, keeping that behavior after a + rebase). A 201 acknowledged during the gap must therefore never be silently + dropped — the scope sits in the store but no resync would ever sync it, so + only the replayed trigger can make it servable. The during-outage scope uses + a *different* seeded repo than (c)'s: serving reads the on-disk clone keyed + by repo URL, so sharing (c)'s repo would let (d) return 200 off the clone + (c)'s sync produced, without (d)'s own trigger ever being delivered. """ opal = opal_multiworker + assert repo_count >= 2, "needs --boot-scopes >= 2" + post_bounce_repo, during_bounce_repo = list_seeded_repos(2) before = worker_pids() assert ( @@ -117,7 +151,16 @@ def test_server_recovers_after_postgres_bounce(opal_multiworker, repo_count): # baseline reader (re)connect count — assertion (b) requires it to increase before_connects = broadcaster_connect_count() - bounce_postgres(down_seconds=5) + def _publish_during_outage(): + # A PUT while the backbone is down: scopes.put persists to Redis, and + # the sync publish is buffered for replay (the broadcaster's outbound + # buffer catches the failed backbone share rather than raising), so + # this returns 201 mid-outage. If it ever starts failing here instead, + # that is a behavior change worth a loud test failure — bounce_postgres + # restores Postgres before propagating the exception. + opal.put_scope("during-bounce", gitea_repo_url(during_bounce_repo)) + + bounce_postgres(down_seconds=5, during=_publish_during_outage) # HTTP must come back first. A respawn would also satisfy this — hence the # PID check below, which a respawn would *not* satisfy. @@ -168,22 +211,20 @@ def test_server_recovers_after_postgres_bounce(opal_multiworker, repo_count): # cloned the repo after the backbone returned. We assert on a served bundle # rather than /internal cache counts, which are per-process and so not # deterministic to read on a 2-worker stack. - healthy = list_seeded_repos(1)[0] - opal.put_scope("post-bounce", gitea_repo_url(healthy)) - served = False - last = None - deadline = time.time() + 120 - while time.time() < deadline: - try: - resp = opal.get_scope_policy("post-bounce") - last = resp.status_code - if resp.status_code == 200: - served = True - break - except requests.RequestException as exc: - last = repr(exc) - time.sleep(2) + opal.put_scope("post-bounce", gitea_repo_url(post_bounce_repo)) + served, last = _wait_served(opal, "post-bounce", timeout=120) assert served, ( f"scope PUT after the bounce never became servable (last: {last}); " f"the broadcaster/sync path did not recover" ) + + # (d) the scope acknowledged DURING the outage must become servable: its + # buffered sync trigger has to be replayed to the leader once the backbone + # is back. By this point (c) has proven the post-recovery path works, so a + # failure here isolates the disconnect-window publish being dropped. + served, last = _wait_served(opal, "during-bounce", timeout=120) + assert served, ( + f"scope PUT during the bounce never became servable (last: {last}); " + f"its sync trigger was dropped instead of buffered/replayed across " + f"the backbone gap" + ) diff --git a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py index 4abd1abb1..87e392910 100644 --- a/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py +++ b/packages/opal-server/opal_server/tests/debug_stats_endpoint_test.py @@ -1,6 +1,10 @@ +import contextlib + from fastapi import Depends, FastAPI, HTTPException from fastapi.testclient import TestClient +from opal_server.config import opal_server_config from opal_server.debug_stats import register_internal_stats_route +from opal_server.server import OpalServer def _app_with_flag(enabled: bool) -> FastAPI: @@ -38,3 +42,45 @@ def _deny(): register_internal_stats_route(app, enabled=True, dependencies=[Depends(_deny)]) resp = TestClient(app).get("/internal/git-fetcher-cache-stats") assert resp.status_code == 401 + + +@contextlib.contextmanager +def _override_config(**overrides): + saved = {key: getattr(opal_server_config, key) for key in overrides} + try: + for key, value in overrides.items(): + setattr(opal_server_config, key, value) + yield + finally: + for key, value in saved.items(): + setattr(opal_server_config, key, value) + + +def _build_server() -> OpalServer: + return OpalServer( + init_policy_watcher=False, + broadcaster_uri=None, + enable_jwks_endpoint=False, + ) + + +def test_server_wiring_mounts_endpoint_when_flag_enabled(): + """The real OpalServer app mounts the route when DEBUG_INTERNAL_STATS is + on. + + The tests above exercise register_internal_stats_route() on a bare + FastAPI app; this one covers the wiring in server.py itself — + without it, removing the register_internal_stats_route() call from + _init_fast_api_app would leave every unit test green. + """ + with _override_config(DEBUG_INTERNAL_STATS=True): + client = TestClient(_build_server().app) + resp = client.get("/internal/git-fetcher-cache-stats") + assert resp.status_code == 200 + assert set(resp.json()) == {"repo_locks", "repos", "repos_last_fetched", "rss_kb"} + + +def test_server_wiring_omits_endpoint_when_flag_disabled(): + with _override_config(DEBUG_INTERNAL_STATS=False): + client = TestClient(_build_server().app) + assert client.get("/internal/git-fetcher-cache-stats").status_code == 404