From 200fcada3d281b01e0f7ca0603ee7282a52142fc Mon Sep 17 00:00:00 2001 From: Aspirant200715 Date: Sat, 31 Oct 2026 23:40:59 +0530 Subject: [PATCH 1/4] feat(deploy): O10 worker VM, restricted database role, locked-down api host, /healthz - deploy/worker-vm: compose file, runbook, env template and preflight for the agent worker on its own VM - worker: refuses a second process with the same WORKER_ID, can require every agent key, sweeps abandoned workspaces, warns when named after its hostname - deploy/worker-vm/db-role.sql: a mergit_worker role that can touch only jobs, pr_requests, plans and tasks - api: GET /healthz (database check), compose healthcheck, update.sh waits for it - Caddy: api.mergit.site serves browser routes and /healthz; /v0/internal only from WORKER_VM_IP - cp_postgres published on CP_DB_BIND (loopback by default) --- .gitignore | 4 + control-plane/mergit_cp/app.py | 3 +- control-plane/mergit_cp/health.py | 37 ++++ control-plane/mergit_worker/__main__.py | 34 ++++ control-plane/mergit_worker/config.py | 3 + control-plane/mergit_worker/startup.py | 109 ++++++++++++ control-plane/tests/test_caddy_routes.py | 68 +++++++- control-plane/tests/test_health.py | 85 +++++++++ control-plane/tests/test_startup.py | 182 +++++++++++++++++++ control-plane/tests/test_worker_role.py | 213 +++++++++++++++++++++++ deploy/Caddyfile | 32 +++- deploy/compose.controlplane.yaml | 12 ++ deploy/compose.server.yaml | 4 + deploy/server/README.md | 3 +- deploy/server/update.sh | 13 ++ deploy/worker-vm/README.md | 62 +++++++ deploy/worker-vm/compose.yaml | 45 +++++ deploy/worker-vm/create-db-role.sh | 33 ++++ deploy/worker-vm/db-role.sql | 25 +++ deploy/worker-vm/preflight.sh | 71 ++++++++ deploy/worker-vm/worker.env.example | 30 ++++ 21 files changed, 1060 insertions(+), 8 deletions(-) create mode 100644 control-plane/mergit_cp/health.py create mode 100644 control-plane/mergit_worker/startup.py create mode 100644 control-plane/tests/test_health.py create mode 100644 control-plane/tests/test_startup.py create mode 100644 control-plane/tests/test_worker_role.py create mode 100644 deploy/worker-vm/README.md create mode 100644 deploy/worker-vm/compose.yaml create mode 100755 deploy/worker-vm/create-db-role.sh create mode 100644 deploy/worker-vm/db-role.sql create mode 100755 deploy/worker-vm/preflight.sh create mode 100644 deploy/worker-vm/worker.env.example diff --git a/.gitignore b/.gitignore index 2722d0be..9452068d 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,7 @@ id_rsa* # visual brainstorming scratch .superpowers/ + +# The worker VM's secrets and the contract files copied onto it by hand +deploy/worker-vm/worker.env +deploy/worker-vm/deployments/ diff --git a/control-plane/mergit_cp/app.py b/control-plane/mergit_cp/app.py index 542cb6f1..6a98f972 100644 --- a/control-plane/mergit_cp/app.py +++ b/control-plane/mergit_cp/app.py @@ -11,7 +11,7 @@ from mergit_worker.meter import PriceTable -from . import auth, chain_events, oauth, otp, routes_plans, routes_runs, wallet_interest +from . import auth, chain_events, health, oauth, otp, routes_plans, routes_runs, wallet_interest from .agents import load_agents from .config import Settings from .db import migrate, open_pool @@ -81,6 +81,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None]: allow_methods=["GET", "POST", "PATCH"], allow_headers=["Content-Type"], ) + app.include_router(health.router) app.include_router(auth.router) app.include_router(auth.account_router) app.include_router(oauth.router) diff --git a/control-plane/mergit_cp/health.py b/control-plane/mergit_cp/health.py new file mode 100644 index 00000000..001802cf --- /dev/null +++ b/control-plane/mergit_cp/health.py @@ -0,0 +1,37 @@ +"""``GET /healthz``: is this API able to serve, i.e. is its database reachable? + +Used by the compose healthcheck and by deploy/server/update.sh. It is deliberately public and says nothing +beyond ok or not ok: no version, no settings, no error text. +""" + +import asyncio +import logging +from typing import Annotated + +from fastapi import APIRouter, Depends +from fastapi.responses import JSONResponse + +from .db import Pool +from .deps import get_pool + +log = logging.getLogger(__name__) + +router = APIRouter(tags=["health"]) + +# Under a proxy's own health timeout, so a stuck database answers 503 instead of hanging the check. +DB_CHECK_TIMEOUT_S = 3.0 + + +async def _database_answers(pool: Pool) -> None: + async with pool.connection() as conn: + await conn.execute("SELECT 1") + + +@router.get("/healthz", include_in_schema=False) +async def healthz(pool: Annotated[Pool, Depends(get_pool)]) -> JSONResponse: + try: + await asyncio.wait_for(_database_answers(pool), DB_CHECK_TIMEOUT_S) + except Exception: # any failure, including a timeout or an exhausted pool, means "not healthy" + log.warning("healthz: the database did not answer", exc_info=True) + return JSONResponse({"ok": False}, status_code=503) + return JSONResponse({"ok": True}) diff --git a/control-plane/mergit_worker/__main__.py b/control-plane/mergit_worker/__main__.py index c471ead6..e83121f3 100644 --- a/control-plane/mergit_worker/__main__.py +++ b/control-plane/mergit_worker/__main__.py @@ -23,6 +23,7 @@ from .providers import llm_provider from .publish import ReceiptPublisher from .sandbox import Sandbox, SandboxConfig, reap_stale +from .startup import WorkerIdLock, agents_without_key, sweep_workspaces from .tools import ToolLimits from .worker import Worker, WorkerDeps from .workspace import GitLimits @@ -141,6 +142,34 @@ def main() -> int: return 2 worker_id = cfg.worker_id or socket.gethostname() + if not cfg.worker_id: + log.warning( + "WORKER_ID is not set, so this worker is named after its hostname (%s). A container's hostname " + "changes whenever it is recreated, and a restarted worker would then no longer find the jobs and " + "sandbox containers its previous run left. Set WORKER_ID to a fixed name, one per process.", + worker_id, + ) + try: + no_key = agents_without_key(cfg.agents_file) + except ConfigError as exc: + log.error("configuration error: %s", exc) + return 2 + if no_key: + names = ", ".join(f"MERGIT_AGENT_KEY_{i}" for i in no_key) + if cfg.require_all_agent_keys: + log.error("REQUIRE_ALL_AGENT_KEYS is set and these are missing or malformed: %s", names) + return 2 + log.warning( + "no usable key for agent(s) %s: a job for them fails, and the requester loses an attempt", names + ) + lock = WorkerIdLock.acquire(cfg.database_url.get_secret_value(), worker_id) + if lock is None: + log.error( + "another worker process is already running as %r. Starting here would fail its running jobs and " + "remove its sandbox containers. Give each process its own WORKER_ID.", + worker_id, + ) + return 2 log.info("model provider: %s", llm_provider()) built = build_worker(cfg, worker_id) log.info("failed %d jobs this worker left running before a restart", built.jobs.fail_own_running()) @@ -160,6 +189,10 @@ def main() -> int: else: log.info("no GITHUB_TOKEN: pull-request requests will not be handled by this worker") log.info("removed %d stale sandbox containers", reap_stale(worker_id)) + log.info( + "removed %d abandoned workspace directories", + sweep_workspaces(cfg.sandbox_workspace_root, cfg.job_lease_s), + ) stop = threading.Event() @@ -172,6 +205,7 @@ def request_stop(_signum: int, _frame: object) -> None: built.worker.run_forever(stop) finally: built.close() + lock.close() return 0 diff --git a/control-plane/mergit_worker/config.py b/control-plane/mergit_worker/config.py index 8380e117..487e9d63 100644 --- a/control-plane/mergit_worker/config.py +++ b/control-plane/mergit_worker/config.py @@ -63,6 +63,9 @@ class WorkerConfig(BaseSettings): # Under the 1800 s dispatch deadline, leaving time to run Φ, seal and publish. job_deadline_s: float = Field(default=1500.0, gt=0) worker_id: str | None = None # defaults to the hostname; one process per id + # Refuse to start unless every agent in the agents file has a MERGIT_AGENT_KEY_. Off by default: a + # worker may deliberately serve only some of the agents (startup.agents_without_key logs the rest). + require_all_agent_keys: bool = False # Opus 5.5's thinking tokens count against this; 8192 made a truncated turn too likely. model_max_tokens: int = Field(default=16_000, ge=256) poll_interval_s: float = Field(default=2.0, gt=0) diff --git a/control-plane/mergit_worker/startup.py b/control-plane/mergit_worker/startup.py new file mode 100644 index 00000000..6402f973 --- /dev/null +++ b/control-plane/mergit_worker/startup.py @@ -0,0 +1,109 @@ +"""Start-up checks and housekeeping for one worker process (plan task O10). + +The worker runs on its own VM, restarted by Docker and redeployed by hand, so what it leaves behind when it +stops has to be cleaned up by the next start: jobs, sandbox containers (sandbox.reap_stale) and workspace +directories (here). Everything below is a plain function over a path, an environment or a database URL, so it +is tested without a Docker daemon. +""" + +from __future__ import annotations + +import json +import logging +import os +import re +import shutil +import time +from collections.abc import Mapping +from pathlib import Path + +import psycopg + +from .config import ConfigError + +log = logging.getLogger(__name__) + +_AGENT_KEY = re.compile(r"^0x[0-9a-fA-F]{64}$") +WORKSPACE_PREFIX = "ws-" # workspace.Workspace.clone names its directories ws- + + +def agents_without_key(agents_file: Path, environ: Mapping[str, str] | None = None) -> list[str]: + """Agent ids in the agents file that have no usable ``MERGIT_AGENT_KEY_`` here. + + The key is read when a job for that agent starts, so a missing one is otherwise found only after a + requester has picked the agent, funded the plan and burned an attempt on it. + """ + env = os.environ if environ is None else environ + try: + registry: list[dict[str, object]] = json.loads(agents_file.read_text())["agents"] + except (OSError, KeyError, ValueError) as exc: + raise ConfigError(f"cannot read agent configuration: {exc}") from exc + ids = [str(a["agent_id"]) for a in registry if "agent_id" in a] + return [i for i in ids if not _AGENT_KEY.match(env.get(f"MERGIT_AGENT_KEY_{i}", ""))] + + +def _remove_workspace(path: Path) -> None: + """Delete one workspace tree, including directories a sandbox left unreadable. + + Never follows a symlink: a sandboxed agent can plant one, and chmod or rmtree through it would reach + outside the workspace root. + """ + for dirpath, dirnames, _files in os.walk(path, followlinks=False): + os.chmod(dirpath, 0o700) + for name in dirnames: + child = os.path.join(dirpath, name) + if not os.path.islink(child): + os.chmod(child, 0o700) + shutil.rmtree(path) + + +def sweep_workspaces(root: Path, older_than_s: float, *, now: float | None = None) -> int: + """Remove ``ws-*`` directories under ``root`` that nothing can still be using. Returns how many. + + A crash or a SIGKILL skips the normal clean-up, and ``reap_stale`` removes containers only, so without + this the VM's disk fills one abandoned checkout at a time. ``older_than_s`` must exceed the longest a + job can run (the lease, which is also when its row is reclaimed), so a job still in flight in another + process on this host is never touched. + """ + if not root.is_dir(): + return 0 + cutoff = (time.time() if now is None else now) - older_than_s + removed = 0 + for entry in root.iterdir(): + if not entry.name.startswith(WORKSPACE_PREFIX) or entry.is_symlink() or not entry.is_dir(): + continue + try: + if entry.stat().st_mtime >= cutoff: + continue + _remove_workspace(entry) + removed += 1 + except OSError as exc: + log.warning("could not remove stale workspace %s: %s", entry, exc) + return removed + + +class WorkerIdLock: + """A session-level Postgres advisory lock named for the worker id, held while the process lives. + + A worker id must belong to one process. At start-up a worker fails every job still ``running`` under its + id and removes every sandbox container labelled with it, so a second process sharing the id would + destroy the first one's live work. Postgres releases the lock when the connection ends, whether the + process exits cleanly or is killed, so a crashed worker never blocks its own restart. + """ + + def __init__(self, conn: psycopg.Connection[tuple[object, ...]]) -> None: + self._conn = conn + + @classmethod + def acquire(cls, database_url: str, worker_id: str) -> WorkerIdLock | None: + conn = psycopg.connect(database_url, autocommit=True) # pyright: ignore[reportArgumentType] + row = conn.execute( + "SELECT pg_try_advisory_lock(hashtextextended(%s, 0))", (f"mergit_worker:{worker_id}",) + ).fetchone() + if row is not None and row[0] is True: + return cls(conn) + conn.close() + return None + + def close(self) -> None: + self._conn.close() diff --git a/control-plane/tests/test_caddy_routes.py b/control-plane/tests/test_caddy_routes.py index bd2a97f0..faa3481d 100644 --- a/control-plane/tests/test_caddy_routes.py +++ b/control-plane/tests/test_caddy_routes.py @@ -17,11 +17,21 @@ INTERNAL = ("/v0/internal", "/v0/chain-events") +def _block(host: str) -> str: + text = CADDYFILE.read_text() + match = re.search(rf"^{re.escape(host)} \{{\n(.*?)\n\}}$", text, re.S | re.M) + assert match, f"no {host} block in the Caddyfile" + return match.group(1) + + def _proxied_patterns() -> list[str]: + """The browser routes: one shared snippet, imported by both app.mergit.site and api.mergit.site.""" text = CADDYFILE.read_text() - block = re.search(r"app\.mergit\.site \{.*?@api path ([^\n]+)", text, re.S) - assert block, "the app.mergit.site block has no `@api path` matcher" - return block.group(1).split() + snippet = re.search(r"^\(api_routes\) \{\n\s*@api path ([^\n]+)\n\}", text, re.M) + assert snippet, "the (api_routes) snippet has no `@api path` matcher" + for host in ("app.mergit.site", "api.mergit.site"): + assert "import api_routes" in _block(host), f"{host} must import the shared route list" + return snippet.group(1).split() def _browser_paths() -> list[str]: @@ -55,3 +65,55 @@ def test_the_internal_routes_stay_off_the_public_origin() -> None: def test_the_check_would_notice_a_route_it_does_not_know() -> None: assert not any(fnmatch("/v0/brand-new/thing", pat) for pat in _proxied_patterns()) + + +def test_the_api_host_serves_only_what_it_means_to() -> None: + """api.mergit.site used to proxy everything, so the worker's internal route and the API's docs pages + were reachable from anywhere. Now: the browser routes, /healthz, and the internal route from the + worker VM only.""" + block = _block("api.mergit.site") + assert "handle @api {" in block and "handle /healthz {" in block + assert re.search(r"handle \{\s*respond \"not found\" 404\s*\}", block), "the catch-all must be a 404" + assert not re.search(r"handle \{\s*reverse_proxy", block), "a catch-all reverse_proxy exposes every route" + + +def test_the_internal_route_is_proxied_only_from_the_worker_vm() -> None: + block = _block("api.mergit.site") + matcher = re.search(r"@worker \{(.*?)\}", block, re.S) + assert matcher, "no @worker matcher" + assert "path /v0/internal/*" in matcher.group(1) + assert "remote_ip {$WORKER_VM_IP:" in matcher.group(1), "the internal route must be limited by IP" + assert "handle @worker" in block + + +def test_the_worker_vm_default_address_is_one_nothing_can_use() -> None: + """If WORKER_VM_IP is unset the rule must admit nobody. 192.0.2.0/24 is reserved for documentation.""" + default = re.search(r"remote_ip \{\$WORKER_VM_IP:([^}]+)\}", CADDYFILE.read_text()) + assert default and default.group(1).startswith("192.0.2."), "the fallback must be a documentation address" + compose = (CADDYFILE.parent / "compose.server.yaml").read_text() + assert "WORKER_VM_IP: ${WORKER_VM_IP:-192.0.2.1}" in compose, ( + "compose must pass the same unusable default" + ) + + +def test_chain_events_is_never_reachable_through_caddy() -> None: + """Only the gateway calls it, over the compose network. Comments may mention it; configuration may not.""" + config = "\n".join( + line for line in CADDYFILE.read_text().splitlines() if not line.lstrip().startswith("#") + ) + assert "chain-events" not in config + + +def test_every_internal_route_can_reach_the_api_from_the_worker_vm() -> None: + """A new worker-facing route must not end up proxied nowhere. Today: /v0/internal/progress.""" + block = _block("api.mergit.site") + pattern = re.search(r"path (/v0/internal/\*)", block) + assert pattern + internal_paths = { + re.sub(r"\{[^}]+\}", "x", r.path) + for r in routes_runs.internal_router.routes + if hasattr(r, "path") # pyright: ignore[reportUnknownArgumentType] + } # pyright: ignore[reportAttributeAccessIssue,reportUnknownMemberType,reportUnknownVariableType] + assert internal_paths, "the internal router has no routes?" + uncovered = [p for p in internal_paths if not fnmatch(p, pattern.group(1))] + assert uncovered == [], f"worker-facing routes the Caddy rule does not cover: {uncovered}" diff --git a/control-plane/tests/test_health.py b/control-plane/tests/test_health.py new file mode 100644 index 00000000..c1535e8a --- /dev/null +++ b/control-plane/tests/test_health.py @@ -0,0 +1,85 @@ +"""/healthz tells a proxy or a deploy script whether this API can serve (its database answers).""" + +from __future__ import annotations + +import asyncio +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager +from typing import Any, cast + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from mergit_cp import health + +pytestmark = pytest.mark.postgres + + +def test_healthz_is_ok_when_the_database_answers(client: TestClient) -> None: + response = client.get("/healthz") + assert response.status_code == 200 + assert response.json() == {"ok": True} + + +def test_healthz_needs_no_session_and_leaks_nothing(client: TestClient) -> None: + body = client.get("/healthz").text + for word in ("version", "postgres", "password", "secret", "token"): + assert word not in body.lower() + + +class _Conn: + def __init__(self, hang: bool) -> None: + self._hang = hang + + async def execute(self, _sql: str) -> None: + if self._hang: + await asyncio.sleep(60) + raise OSError("connection refused") + + +class _BrokenPool: + def __init__(self, hang: bool = False) -> None: + self._hang = hang + + @asynccontextmanager + async def connection(self) -> AsyncIterator[_Conn]: + yield _Conn(self._hang) + + +def _app_with(pool: object) -> FastAPI: + app = FastAPI() + app.state.pool = pool + app.include_router(health.router) + return app + + +def test_healthz_is_503_when_the_database_errors() -> None: + with TestClient(_app_with(_BrokenPool())) as c: + response = c.get("/healthz") + assert response.status_code == 503 + assert response.json() == {"ok": False} + assert "refused" not in response.text # the error text stays in the log + + +def test_healthz_is_503_not_a_hang_when_the_database_stalls(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(health, "DB_CHECK_TIMEOUT_S", 0.2) + with TestClient(_app_with(_BrokenPool(hang=True))) as c: + response = c.get("/healthz") + assert response.status_code == 503 + + +def test_the_database_probe_is_one_select(monkeypatch: pytest.MonkeyPatch) -> None: + seen: list[str] = [] + + class Conn: + async def execute(self, sql: str) -> None: + seen.append(sql) + + class Pool: + @asynccontextmanager + async def connection(self) -> AsyncIterator[Conn]: + yield Conn() + + asyncio.run(health._database_answers(cast(Any, Pool()))) + assert seen == ["SELECT 1"] diff --git a/control-plane/tests/test_startup.py b/control-plane/tests/test_startup.py new file mode 100644 index 00000000..23482e85 --- /dev/null +++ b/control-plane/tests/test_startup.py @@ -0,0 +1,182 @@ +"""Start-up checks and housekeeping for a worker that lives on its own VM (plan task O10).""" + +from __future__ import annotations + +import json +import os +from pathlib import Path + +import pytest + +from mergit_worker.config import ConfigError +from mergit_worker.startup import agents_without_key, sweep_workspaces + +KEY_A = "0x" + "a" * 64 +KEY_B = "0x" + "b" * 64 + + +def agents_file(tmp_path: Path, *ids: str) -> Path: + path = tmp_path / "agents.json" + path.write_text(json.dumps({"agents": [{"agent_id": i, "name": f"agent-{i}"} for i in ids]})) + return path + + +# -- agents_without_key -------------------------------------------------------------------------------- + + +def test_every_agent_with_a_key_means_nothing_is_missing(tmp_path: Path) -> None: + env = {"MERGIT_AGENT_KEY_1": KEY_A, "MERGIT_AGENT_KEY_2": KEY_B} + assert agents_without_key(agents_file(tmp_path, "1", "2"), env) == [] + + +def test_an_agent_with_no_key_is_reported(tmp_path: Path) -> None: + assert agents_without_key(agents_file(tmp_path, "1", "2"), {"MERGIT_AGENT_KEY_1": KEY_A}) == ["2"] + + +@pytest.mark.parametrize( + "bad", + ["", "0x1234", "0x" + "g" * 64, KEY_A[2:], KEY_A + "00", " " + KEY_A], + ids=["empty", "short", "not-hex", "no-0x", "too-long", "leading-space"], +) +def test_a_malformed_key_counts_as_missing(tmp_path: Path, bad: str) -> None: + assert agents_without_key(agents_file(tmp_path, "1"), {"MERGIT_AGENT_KEY_1": bad}) == ["1"] + + +def test_a_key_for_another_agent_does_not_cover_this_one(tmp_path: Path) -> None: + assert agents_without_key(agents_file(tmp_path, "10"), {"MERGIT_AGENT_KEY_1": KEY_A}) == ["10"] + + +def test_an_unreadable_agents_file_is_a_config_error_not_a_crash(tmp_path: Path) -> None: + with pytest.raises(ConfigError): + agents_without_key(tmp_path / "missing.json", {}) + broken = tmp_path / "broken.json" + broken.write_text("{not json") + with pytest.raises(ConfigError): + agents_without_key(broken, {}) + wrong_shape = tmp_path / "shape.json" + wrong_shape.write_text('{"not_agents": []}') + with pytest.raises(ConfigError): + agents_without_key(wrong_shape, {}) + + +def test_reads_the_process_environment_by_default(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("MERGIT_AGENT_KEY_7", KEY_A) + assert agents_without_key(agents_file(tmp_path, "7")) == [] + + +# -- sweep_workspaces ---------------------------------------------------------------------------------- + +HOUR = 3600.0 + + +def workspace(root: Path, name: str, age_s: float, now: float) -> Path: + ws = root / name + (ws / "work").mkdir(parents=True) + (ws / "git").mkdir() + (ws / "work" / "file.txt").write_text("checkout") + os.utime(ws, (now - age_s, now - age_s)) + return ws + + +def test_an_old_workspace_is_removed_and_a_recent_one_is_kept(tmp_path: Path) -> None: + now = 1_000_000.0 + old = workspace(tmp_path, "ws-old", 2 * HOUR, now) + fresh = workspace(tmp_path, "ws-fresh", 60, now) + assert sweep_workspaces(tmp_path, HOUR, now=now) == 1 + assert not old.exists() + assert fresh.exists() + + +def test_a_workspace_exactly_at_the_limit_is_kept(tmp_path: Path) -> None: + # A job still inside its lease must never lose its checkout. + now = 1_000_000.0 + edge = workspace(tmp_path, "ws-edge", HOUR, now) + assert sweep_workspaces(tmp_path, HOUR, now=now) == 0 + assert edge.exists() + + +def test_only_workspace_directories_are_touched(tmp_path: Path) -> None: + now = 1_000_000.0 + other_dir = tmp_path / "keep-me" + other_dir.mkdir() + loose_file = tmp_path / "ws-not-a-directory" + loose_file.write_text("x") + for p in (other_dir, loose_file): + os.utime(p, (now - 5 * HOUR, now - 5 * HOUR)) + assert sweep_workspaces(tmp_path, HOUR, now=now) == 0 + assert other_dir.exists() and loose_file.exists() + + +def test_a_missing_root_is_nothing_to_do(tmp_path: Path) -> None: + assert sweep_workspaces(tmp_path / "does-not-exist", HOUR) == 0 + + +def test_a_workspace_a_sandbox_left_unreadable_is_still_removed(tmp_path: Path) -> None: + now = 1_000_000.0 + ws = workspace(tmp_path, "ws-locked", 2 * HOUR, now) + locked = ws / "work" / "locked" + locked.mkdir() + (locked / "inner.txt").write_text("x") + locked.chmod(0o000) + try: + assert sweep_workspaces(tmp_path, HOUR, now=now) == 1 + finally: + if locked.exists(): + locked.chmod(0o700) + assert not ws.exists() + + +def test_a_planted_symlink_is_removed_but_never_followed(tmp_path: Path) -> None: + """A sandboxed agent controls the contents of its workspace. A link to a directory elsewhere must not + make the sweep chmod or delete what it points at.""" + now = 1_000_000.0 + outside = tmp_path / "outside" + outside.mkdir() + (outside / "precious.txt").write_text("keep") + outside.chmod(0o755) + root = tmp_path / "root" + root.mkdir() + ws = workspace(root, "ws-evil", 2 * HOUR, now) + (ws / "work" / "escape").symlink_to(outside, target_is_directory=True) + (ws / "work" / "dangling").symlink_to(tmp_path / "nowhere") + + assert sweep_workspaces(root, HOUR, now=now) == 1 + + assert not ws.exists() + assert (outside / "precious.txt").read_text() == "keep" + assert outside.stat().st_mode & 0o777 == 0o755 + + +def test_a_workspace_root_that_is_itself_a_symlink_entry_is_skipped(tmp_path: Path) -> None: + now = 1_000_000.0 + real = tmp_path / "elsewhere" + real.mkdir() + os.utime(real, (now - 5 * HOUR, now - 5 * HOUR)) + root = tmp_path / "root" + root.mkdir() + (root / "ws-link").symlink_to(real, target_is_directory=True) + assert sweep_workspaces(root, HOUR, now=now) == 0 + assert real.exists() + + +def test_a_removal_failure_is_logged_and_the_rest_still_go( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + now = 1_000_000.0 + stuck = workspace(tmp_path, "ws-a-stuck", 2 * HOUR, now) + other = workspace(tmp_path, "ws-b-other", 2 * HOUR, now) + + import mergit_worker.startup as startup + + real = startup._remove_workspace + + def flaky(path: Path) -> None: + if path.name == "ws-a-stuck": + raise PermissionError("busy") + real(path) + + monkeypatch.setattr(startup, "_remove_workspace", flaky) + with caplog.at_level("WARNING"): + assert sweep_workspaces(tmp_path, HOUR, now=now) == 1 + assert stuck.exists() and not other.exists() + assert "ws-a-stuck" in caplog.text diff --git a/control-plane/tests/test_worker_role.py b/control-plane/tests/test_worker_role.py new file mode 100644 index 00000000..7140d3fc --- /dev/null +++ b/control-plane/tests/test_worker_role.py @@ -0,0 +1,213 @@ +"""The worker VM connects to the control-plane database as its own least-privilege role (plan task O10). + +``deploy/worker-vm/db-role.sql`` is the grant list. These tests run it as shipped against a database built by +the real migrations, then act as that role: every statement the worker issues must work, and nothing else may. +""" + +from __future__ import annotations + +import asyncio +from collections.abc import Iterator +from pathlib import Path + +import psycopg +import pytest +from psycopg import errors +from psycopg.conninfo import make_conninfo + +from mergit_cp.db import migrate +from mergit_cp.pr_store import _CHANGES, PgPrStore +from mergit_worker import jobs as worker_jobs +from mergit_worker.jobs import PgJobStore +from mergit_worker.startup import WorkerIdLock + +pytestmark = pytest.mark.postgres + +GRANTS = Path(__file__).resolve().parents[2] / "deploy" / "worker-vm" / "db-role.sql" +ROLE = "mergit_worker" +PASSWORD = "role-test-password" +DB = "worker_role_test" +TASK = "0x" + "0" * 64 +PLAN = "00000000-0000-0000-0000-000000000000" +WORKER_TABLES = {"jobs", "pr_requests", "plans", "tasks"} +SENSITIVE = {"sessions", "users", "identities", "wallets", "email_otps", "oauth_states", "wallet_interest"} + + +async def _migrate(url: str) -> None: + async with await psycopg.AsyncConnection.connect(url) as conn: + await migrate(conn) + + +@pytest.fixture(scope="module") +def admin_url(database_url: str) -> Iterator[str]: + """A throwaway database with every migration applied and the grants file run, as the real script does.""" + with psycopg.connect(database_url, autocommit=True) as root: + root.execute(f"DROP DATABASE IF EXISTS {DB}") + root.execute(f"CREATE DATABASE {DB}") + url = make_conninfo(database_url, dbname=DB) + asyncio.run(_migrate(url)) + with psycopg.connect(database_url, autocommit=True) as root: + root.execute(f"DROP ROLE IF EXISTS {ROLE}") # a previous run's role, owned by nothing now + root.execute(f"CREATE ROLE {ROLE} LOGIN PASSWORD '{PASSWORD}'") + with psycopg.connect(url, autocommit=True) as conn: + conn.execute(GRANTS.read_text()) # pyright: ignore[reportArgumentType] + yield url + with psycopg.connect(database_url, autocommit=True) as root: + root.execute(f"DROP DATABASE {DB} WITH (FORCE)") + root.execute(f"DROP ROLE {ROLE}") + + +@pytest.fixture +def role_url(admin_url: str) -> str: + return make_conninfo(admin_url, user=ROLE, password=PASSWORD) + + +def _tables(admin_url: str) -> set[str]: + with psycopg.connect(admin_url) as conn: + rows = conn.execute("SELECT tablename FROM pg_tables WHERE schemaname = 'public'").fetchall() + return {r[0] for r in rows} + + +# -- everything the worker does works ------------------------------------------------------------------ + + +def test_the_job_queue_methods_work_as_the_role(role_url: str) -> None: + store = PgJobStore.connect(role_url, "role-test") + try: + assert store.fail_own_running() == 0 + assert store.reclaim_stale(3600.0) == 0 + assert store.claim() is None # SELECT ... FOR UPDATE SKIP LOCKED, then UPDATE + store.fail(TASK, "nothing to fail") # UPDATE ... WHERE task_id + finally: + store.close() + + +def test_the_job_loading_queries_work_as_the_role(role_url: str) -> None: + """_LOAD joins plans and tasks, and a dependent task reads its dependencies' patches: both need SELECT.""" + with psycopg.connect(role_url, autocommit=True) as conn: + assert conn.execute(worker_jobs._LOAD, {"task_id": TASK}).fetchall() == [] + assert ( + conn.execute(worker_jobs._DEPENDENCY_PATCHES, {"plan_uuid": PLAN, "indexes": [0]}).fetchall() + == [] + ) + params = {"status": "done", "holds": True, "root": None, "notes": None, "patch": None} + assert conn.execute(worker_jobs._REPORT, {**params, "task_id": TASK, "worker": "w"}).rowcount == 0 + + +def test_the_pull_request_queue_methods_work_as_the_role(role_url: str) -> None: + store = PgPrStore.connect(role_url, "role-test") + try: + assert store.claim() is None + assert store.requeue_own_running() == 0 + assert store.reclaim_stale(3600.0) == 0 + assert store.plan(PLAN) is None # SELECT FROM plans + assert store.request(PLAN) is None + store.finish(PLAN, "branch", "a" * 40, 1, "https://example.test/pr/1") + store.fail(PLAN, "nothing to fail") + finally: + store.close() + with psycopg.connect(role_url, autocommit=True) as conn: + assert conn.execute(_CHANGES, {"plan": PLAN}).fetchall() == [] # tasks LEFT JOIN jobs + + +# -- and nothing else does ------------------------------------------------------------------------------ + + +def test_the_sensitive_tables_exist_so_the_next_test_is_not_vacuous(admin_url: str) -> None: + assert _tables(admin_url) >= SENSITIVE + + +def test_the_role_cannot_read_any_table_it_was_not_given(admin_url: str, role_url: str) -> None: + denied = sorted(_tables(admin_url) - WORKER_TABLES) + assert set(denied) >= SENSITIVE + with psycopg.connect(role_url, autocommit=True) as conn: + for table in denied: + with pytest.raises(errors.InsufficientPrivilege): + conn.execute(f'SELECT * FROM "{table}" LIMIT 1') # pyright: ignore[reportArgumentType] + + +@pytest.mark.parametrize( + "sql", + [ + "DELETE FROM jobs", + "DELETE FROM pr_requests", + "INSERT INTO pr_requests (plan_uuid, requested_by) VALUES ('{plan}', 'x')", + "UPDATE plans SET state = 'done'", + "DELETE FROM plans", + "UPDATE tasks SET title = 'x'", + "DELETE FROM tasks", + "TRUNCATE jobs", + "CREATE TABLE stolen (x int)", + "ALTER TABLE jobs ADD COLUMN x int", + "DROP TABLE jobs", + ], + ids=lambda s: s.split(" (")[0], +) +def test_the_role_cannot_change_more_than_the_worker_needs(role_url: str, sql: str) -> None: + with psycopg.connect(role_url, autocommit=True) as conn, pytest.raises(errors.InsufficientPrivilege): + conn.execute(sql.format(plan=PLAN)) # pyright: ignore[reportArgumentType] + + +def test_running_the_grants_again_takes_extra_rights_away(admin_url: str, role_url: str) -> None: + with psycopg.connect(admin_url, autocommit=True) as conn: + conn.execute(f"GRANT SELECT ON sessions TO {ROLE}") + with psycopg.connect(role_url, autocommit=True) as conn: + conn.execute("SELECT 1 FROM sessions LIMIT 1") # the leak the next run must close + with psycopg.connect(admin_url, autocommit=True) as conn: + conn.execute(GRANTS.read_text()) # pyright: ignore[reportArgumentType] + with psycopg.connect(role_url, autocommit=True) as conn, pytest.raises(errors.InsufficientPrivilege): + conn.execute("SELECT 1 FROM sessions LIMIT 1") + + +# -- one process per worker id -------------------------------------------------------------------------- + + +def test_a_second_process_with_the_same_worker_id_is_refused(database_url: str) -> None: + first = WorkerIdLock.acquire(database_url, "dup-id") + assert first is not None + try: + assert WorkerIdLock.acquire(database_url, "dup-id") is None + finally: + first.close() + + +def test_different_worker_ids_do_not_block_each_other(database_url: str) -> None: + a = WorkerIdLock.acquire(database_url, "id-a") + b = WorkerIdLock.acquire(database_url, "id-b") + assert a is not None and b is not None + a.close() + b.close() + + +def test_the_id_is_free_again_once_the_holder_exits(database_url: str) -> None: + first = WorkerIdLock.acquire(database_url, "restart-id") + assert first is not None + first.close() + again = WorkerIdLock.acquire(database_url, "restart-id") + assert again is not None + again.close() + + +def test_a_killed_worker_does_not_block_its_own_restart(database_url: str) -> None: + """A SIGKILLed container never calls close(); Postgres drops the session and with it the lock.""" + dead = WorkerIdLock.acquire(database_url, "killed-id") + assert dead is not None + with psycopg.connect(database_url, autocommit=True) as admin: + admin.execute( + "SELECT pg_terminate_backend(pid) FROM pg_stat_activity" + " WHERE pid <> pg_backend_pid() AND query LIKE '%pg_try_advisory_lock%'" + ) + restarted = None + for _ in range(50): # the server notices the dropped connection almost at once + restarted = WorkerIdLock.acquire(database_url, "killed-id") + if restarted is not None: + break + asyncio.run(asyncio.sleep(0.1)) + assert restarted is not None + restarted.close() + + +def test_the_role_can_take_the_lock_too(role_url: str) -> None: + lock = WorkerIdLock.acquire(role_url, "role-lock-id") + assert lock is not None + lock.close() diff --git a/deploy/Caddyfile b/deploy/Caddyfile index 36f5e008..1b9b63fb 100644 --- a/deploy/Caddyfile +++ b/deploy/Caddyfile @@ -30,12 +30,18 @@ www.mergit.site { redir https://mergit.site{uri} permanent } +# The API routes a browser calls. One list for app.mergit.site and api.mergit.site, so the two cannot drift; +# control-plane/tests/test_caddy_routes.py fails when a route in the API is missing from it. +(api_routes) { + @api path /v0/auth/* /v0/account/* /v0/wallet-interest /v0/plans /v0/plans/* /v0/agents +} + # The same frontend on live Base Sepolia data, and the orchestration API on the same origin: the browser calls # /v0/auth, /v0/account, /v0/wallet-interest, /v0/plans and /v0/agents here, so sign-in cookies are first-party and there is no CORS. The gateway and the # worker reach the API directly over the compose network, so /v0/chain-events and /v0/internal are deliberately not # proxied. app.mergit.site { - @api path /v0/auth/* /v0/account/* /v0/wallet-interest /v0/plans /v0/plans/* /v0/agents + import api_routes handle @api { reverse_proxy api:8000 } @@ -55,8 +61,28 @@ store.mergit.site { reverse_proxy store:8081 } -# The same API on its own name, for anything that is not the web app. Optional: the app uses app.mergit.site above. +# The API on its own name. It serves the browser routes (api_routes) and /healthz, and the one route the agent worker +# calls from its own VM: /v0/internal/progress, only from the IP in WORKER_VM_IP (deploy/.env, passed in by +# compose.server.yaml; the default is a documentation address nothing can use). Everything else is 404: the +# gateway reaches /v0/chain-events over the compose network, and the API's own docs pages are not for the public. +# The worker's bearer token is still checked by the API; the IP rule only keeps the route off the open internet. # Needs an A record for `api`. It sets its own CORS headers (CORS_ORIGINS) and a Secure, HttpOnly session cookie. api.mergit.site { - reverse_proxy api:8000 + import api_routes + @worker { + path /v0/internal/* + remote_ip {$WORKER_VM_IP:192.0.2.1} + } + handle @api { + reverse_proxy api:8000 + } + handle @worker { + reverse_proxy api:8000 + } + handle /healthz { + reverse_proxy api:8000 + } + handle { + respond "not found" 404 + } } diff --git a/deploy/compose.controlplane.yaml b/deploy/compose.controlplane.yaml index 83c16d1e..983bfafd 100644 --- a/deploy/compose.controlplane.yaml +++ b/deploy/compose.controlplane.yaml @@ -16,6 +16,10 @@ services: environment: { POSTGRES_PASSWORD: mergit, POSTGRES_DB: mergit_cp } volumes: ["cp_pgdata:/var/lib/postgresql/data"] healthcheck: { test: ["CMD", "pg_isready", "-U", "postgres"], interval: 2s, retries: 30 } + # Only the worker VM connects from outside (deploy/worker-vm/README.md), as the restricted mergit_worker role. + # Loopback until CP_DB_BIND (deploy/.env) names this server's private-network address. Never 0.0.0.0: Docker's + # published ports skip the host firewall, so the port would be open to the internet. + ports: ["${CP_DB_BIND:-127.0.0.1}:5433:5432"] # The gateway posts chain events to the control plane instead of the host. gateway: @@ -32,6 +36,14 @@ services: cp_postgres: { condition: service_healthy } volumes: - ../../mergit-contracts/deployments/agents-base-sepolia.json:/agents.json:ro + # /healthz answers 200 only while the database does. The slim image has no curl, so python makes the request; + # urlopen raises on a 503, which is a non-zero exit. + healthcheck: + test: ["CMD", "python", "-c", "import urllib.request; urllib.request.urlopen('http://127.0.0.1:8000/healthz', timeout=3)"] + interval: 15s + timeout: 5s + retries: 3 + start_period: 40s environment: DATABASE_URL: postgres://postgres:mergit@cp_postgres:5432/mergit_cp GATEWAY_URL: http://gateway:8082 diff --git a/deploy/compose.server.yaml b/deploy/compose.server.yaml index d92d0143..9ee88c44 100644 --- a/deploy/compose.server.yaml +++ b/deploy/compose.server.yaml @@ -20,6 +20,10 @@ services: - /srv/mergit/sites:/srv:ro - caddy_data:/data - caddy_config:/config + environment: + # The agent worker's VM (deploy/worker-vm/README.md): the only address allowed to call the API's internal + # routes. The default is a documentation address nothing can use, i.e. nobody. + WORKER_VM_IP: ${WORKER_VM_IP:-192.0.2.1} depends_on: [store, gateway] volumes: { caddy_data: {}, caddy_config: {} } diff --git a/deploy/server/README.md b/deploy/server/README.md index 5e2cc814..4315a66b 100644 --- a/deploy/server/README.md +++ b/deploy/server/README.md @@ -10,7 +10,8 @@ One server (Hetzner CPX32, Nuremberg) runs everything that used to run on a lapt | `store.mergit.site` | evidence store (receipt bundles) | Caddy, then `store:8081` | | _(not public)_ | validator, Postgres | compose network only | | `app.mergit.site/v0/{auth,account,plans,agents}` | orchestration API (control plane), same origin as the app | Caddy, then `api:8000`; needs `deploy/compose.controlplane.yaml` | -| `api.mergit.site` | the same API on its own name (optional) | Caddy, then `api:8000`; needs an A record | +| `api.mergit.site` | the same API on its own name: browser routes, `/healthz`, and `/v0/internal/*` from the worker VM only (`WORKER_VM_IP`); everything else 404 | Caddy, then `api:8000`; needs an A record | +| _(its own VM)_ | the agent worker | `deploy/worker-vm/README.md` | Everything is testnet money. The Hetzner firewall (`mergit-fw`) allows only 22, 80 and 443. diff --git a/deploy/server/update.sh b/deploy/server/update.sh index fc42c346..6f2f013b 100755 --- a/deploy/server/update.sh +++ b/deploy/server/update.sh @@ -20,5 +20,18 @@ case "$what" in esac case "$what" in all) deploy/server/build-sites.sh all ;; web|app) deploy/server/build-sites.sh "$what" ;; esac +# `up -d` returns when the containers have started, not when the API has run its start-up migrations, so give it a +# moment instead of failing a good deploy on the first request. +wait_healthy() { # name url [tries] + local tries=${3:-30} + for _ in $(seq "$tries"); do + if curl -fsS -m 5 "$2/healthz" >/dev/null 2>&1; then echo "$1: ok"; return 0; fi + sleep 2 + done + echo "$1 did not become healthy at $2/healthz" >&2 + return 1 +} + compose ps for h in gateway store; do printf '%s: ' "$h"; curl -fsS "https://$h.mergit.site/healthz"; echo; done +wait_healthy api https://api.mergit.site diff --git a/deploy/worker-vm/README.md b/deploy/worker-vm/README.md new file mode 100644 index 00000000..4a99f1ae --- /dev/null +++ b/deploy/worker-vm/README.md @@ -0,0 +1,62 @@ +# The agent worker on its own VM (plan task O10, issue #26) + +The worker starts sandbox containers through the host's Docker socket, and the sandboxes run code a model wrote. Whoever +controls the worker process controls its host, so it gets a VM with nothing else on it. This VM holds its agent keys, +model keys and one restricted database role. It has no gateway key, no treasury key, no web app and no database. + +``` + worker VM ──(private network, :5433, role mergit_worker)──▶ server: cp_postgres + ──https──▶ gateway.mergit.site (receipt.sealed) + ──https──▶ store.mergit.site (receipt bundles) + ──https──▶ api.mergit.site/v0/internal/progress (live steps; allowed only from this VM's IP) +``` + +## 1. On the server (once) + +1. A second Hetzner server in the same project, plus a private network joining both. Note the server's private address. +2. `deploy/.env` (git-ignored; compose reads it): `CP_DB_BIND=` and `WORKER_VM_IP=`. + Leave `CP_DB_BIND` unset and Postgres stays on loopback. Never set it to `0.0.0.0`: Docker's published ports skip the host firewall. +3. Make the role (prints its URL once): `deploy/worker-vm/create-db-role.sh`. It grants only `jobs`, `pr_requests` + (read and update) and `plans`, `tasks` (read). No sessions, users, wallets or email addresses. +4. `docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml -f deploy/compose.server.yaml up -d`. +5. DNS: an A record for `api` to the server (it exists today). The Hetzner firewall stays 22, 80, 443; the private network is not filtered by it. + +## 2. On the worker VM + +```bash +git clone https://github.com/mergit-io/mergit.git && cd mergit/deploy/worker-vm +sudo install -d -o "$USER" /var/lib/mergit/workspaces +docker build -t mergit-sandbox:dev ../../control-plane/sandbox +mkdir deployments && scp server:mergit/mergit-contracts/deployments/{base-sepolia,agents-base-sepolia}.json deployments/ +cp worker.env.example worker.env && chmod 600 worker.env # fill it in: names only in the example, values never committed +docker compose build && ./preflight.sh # every line must say ok +docker compose up -d && docker compose logs -f +``` + +Expected in the log: `failed N jobs`, `reclaimed N jobs`, `removed N stale sandbox containers`, `removed N abandoned workspace directories`, then it polls. + +## 3. Cutting over from the worker on the main server + +The main server still runs a worker in `compose.controlplane.yaml`. Move in this order, so there is never a gap: + +1. Start the VM worker (step 2) and confirm it claims a job: `docker compose logs worker | grep claimed`. +2. Then, on the server: `docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml rm -sf worker`. +3. A follow-up change deletes the `worker` service from `compose.controlplane.yaml` so `update.sh` stops recreating it. Until then a push to + `main` brings the old worker back: run step 2 again, or remove it first. + +Two workers on one queue are safe (`FOR UPDATE SKIP LOCKED`); they must have different `WORKER_ID`s. + +## Things that go wrong + +| Symptom | Cause | +| --- | --- | +| `another worker process is already running as 'x'` | Two processes share a `WORKER_ID`. Each id is one process. A killed worker never blocks its own restart. | +| `REQUIRE_ALL_AGENT_KEYS is set and these are missing` | `agents-base-sepolia.json` lists an agent with no `MERGIT_AGENT_KEY_`. Add it, or the requester loses an attempt on that agent. | +| `WORKER_ID is not set` | The hostname is the container id and changes on every recreate, so the previous run's jobs and sandboxes are not found. Set it. | +| Live steps missing in the app, jobs still finish | The API rejected `/v0/internal/progress`: `WORKER_VM_IP` is wrong or unset. Progress is best-effort. | +| `database refused` in preflight | `CP_DB_BIND` unset, wrong private address, or the password: rerun `create-db-role.sh`. | +| A deploy killed a running job | The job is `error` and the next agent takes over. `stop_grace_period` is 40 minutes so `docker compose stop` and `down` wait for the job. A host reboot does not: drain first (`docker compose stop`), then reboot. | +| Disk filling | `docker system df` and `du -sh /var/lib/mergit/workspaces`. Old workspaces are swept at every start; images are not: `docker image prune`. | + +A dead worker's jobs are reclaimed only when a worker next starts (lease: one hour). Watch that the VM is up: +`docker compose ps` should say `Up`, and `docker compose logs --since 10m` should show polling or claims. diff --git a/deploy/worker-vm/compose.yaml b/deploy/worker-vm/compose.yaml new file mode 100644 index 00000000..7560764b --- /dev/null +++ b/deploy/worker-vm/compose.yaml @@ -0,0 +1,45 @@ +# The agent worker on its own VM (plan task O10). Runbook: deploy/worker-vm/README.md. +# cd deploy/worker-vm && ./preflight.sh && docker compose up -d --build +# The build context is the repository root (mergit_receipt is a path dependency on ../runtime/py), so run this from +# a full checkout of this repository, not a copy of this folder. +# +# Why its own machine: the worker starts sandbox containers through the host's Docker socket, and the sandboxes run +# code a model wrote. Whoever controls this process controls this host. Here that is a VM with nothing else on it: +# no database, no gateway key, no treasury key. What it holds is its agent keys, model keys and one restricted +# database role (db-role.sql), all in worker.env, which is copied by hand and never committed. + +services: + worker: + build: { context: ../.., dockerfile: control-plane/worker.Dockerfile } + image: mergit-worker:local + restart: unless-stopped + init: true + env_file: worker.env + environment: + REQUIRE_ALL_AGENT_KEYS: "true" + GATEWAY_URL: https://gateway.mergit.site + STORE_URL: https://store.mergit.site + CONTROL_PLANE_URL: https://api.mergit.site + DEPLOYMENT_FILE: /deployments/base-sepolia.json + AGENTS_FILE: /agents.json + SANDBOX_IMAGE: mergit-sandbox:dev + SANDBOX_WORKSPACE_ROOT: /var/lib/mergit/workspaces + # A job runs for up to job_deadline_s (25 minutes) plus checking and publishing. On SIGTERM the worker finishes + # the job it is on and then exits, so give it longer than any job. Docker's default of 10 seconds would SIGKILL + # it mid-job: the job stays `running` until its lease expires and the requester loses an attempt. + stop_grace_period: 40m + stop_signal: SIGTERM + security_opt: ["no-new-privileges:true"] + # A worker that logs a lot must not fill the VM's disk. + logging: + driver: json-file + options: { max-size: "20m", max-file: "5" } + volumes: + - /var/run/docker.sock:/var/run/docker.sock + # The identical path on the host and in the container: sandboxes are started through the host daemon, which + # resolves this bind source itself (control-plane/README.md). + - /var/lib/mergit/workspaces:/var/lib/mergit/workspaces + # Copied from mergit-contracts/deployments by hand (README step 4). Missing files make Docker create empty + # DIRECTORIES here, so preflight.sh checks they are real files first. + - ./deployments/base-sepolia.json:/deployments/base-sepolia.json:ro + - ./deployments/agents-base-sepolia.json:/agents.json:ro diff --git a/deploy/worker-vm/create-db-role.sh b/deploy/worker-vm/create-db-role.sh new file mode 100755 index 00000000..389ba253 --- /dev/null +++ b/deploy/worker-vm/create-db-role.sh @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +# Creates the worker VM's database role (mergit_worker) with only the rights in db-role.sql, or resets its password. +# deploy/worker-vm/create-db-role.sh # makes a password and prints it once +# WORKER_DB_PASSWORD=... deploy/worker-vm/create-db-role.sh # uses one you chose (A-Z a-z 0-9 . _ ~ -, 24+ long) +# Run it on the server that runs cp_postgres, after the control plane has started once (it creates the tables). +# Safe to run again: it re-sets the password and takes back any right that is not in db-role.sql. +# PSQL replaces how psql is reached, for testing this script against a throwaway database. +set -euo pipefail +cd "$(dirname "$0")/../.." + +pw=${WORKER_DB_PASSWORD:-$(head -c 24 /dev/urandom | od -An -tx1 | tr -d ' \n')} +[[ $pw =~ ^[A-Za-z0-9._~-]{24,}$ ]] || { echo "WORKER_DB_PASSWORD must be 24 or more of A-Z a-z 0-9 . _ ~ -" >&2; exit 1; } + +if [ -n "${PSQL:-}" ]; then + read -ra psql <<<"$PSQL" +else + psql=(docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml + exec -T cp_postgres psql -U postgres -d mergit_cp -v ON_ERROR_STOP=1 -q) +fi + +# The password travels on stdin, never on a command line, where `ps` would show it. +{ + printf "\\\\set pw '%s'\n" "$pw" + cat <<'SQL' +SELECT format('CREATE ROLE mergit_worker LOGIN PASSWORD %L', :'pw') + WHERE NOT EXISTS (SELECT FROM pg_roles WHERE rolname = 'mergit_worker') \gexec +SELECT format('ALTER ROLE mergit_worker LOGIN PASSWORD %L', :'pw') \gexec +SQL + cat deploy/worker-vm/db-role.sql +} | "${psql[@]}" + +echo "role mergit_worker is ready. Put this in the worker VM's env file (use the private address, not a public one):" +echo "DATABASE_URL=postgres://mergit_worker:${pw}@:5433/mergit_cp?sslmode=prefer" diff --git a/deploy/worker-vm/db-role.sql b/deploy/worker-vm/db-role.sql new file mode 100644 index 00000000..27656887 --- /dev/null +++ b/deploy/worker-vm/db-role.sql @@ -0,0 +1,25 @@ +-- What the worker VM's database role may do. Idempotent; run as a superuser against the control-plane database +-- AFTER the role exists (create-db-role.sh creates it and then runs this file). +-- +-- The worker reads and writes the job queue directly (mergit_worker/jobs.py) and the pull-request queue +-- (mergit_cp/pr_store.py), and reads the plan and task it is working on. Nothing else. In particular it gets no +-- access to sessions, users, identities, wallets, email_otps, oauth_states or wallet_interest: the worker VM +-- runs model-written code and holds the Docker socket, so it is the machine most likely to be compromised, and +-- a stolen worker credential must not expose sign-in sessions or email addresses. +-- +-- If a new worker query needs another table, add it here AND in tests/test_worker_role.py, which runs every +-- statement the worker issues as this role. + +DO $$ +BEGIN + EXECUTE format('GRANT CONNECT ON DATABASE %I TO mergit_worker', current_database()); +END +$$; + +GRANT USAGE ON SCHEMA public TO mergit_worker; + +-- Start from nothing, so re-running after a tightening takes the old rights away. +REVOKE ALL ON ALL TABLES IN SCHEMA public FROM mergit_worker; + +GRANT SELECT, UPDATE ON jobs, pr_requests TO mergit_worker; +GRANT SELECT ON plans, tasks TO mergit_worker; diff --git a/deploy/worker-vm/preflight.sh b/deploy/worker-vm/preflight.sh new file mode 100755 index 00000000..60a2b771 --- /dev/null +++ b/deploy/worker-vm/preflight.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# Checks the worker VM is ready, before `docker compose up`. One line per check; exits non-zero if any failed. +# cd deploy/worker-vm && ./preflight.sh +# It reads worker.env as text and never runs it, and never prints a secret. +# API_URL / GATEWAY_URL / STORE_URL override the hosted addresses (for a staging stack). +set -uo pipefail +cd "$(dirname "$0")" +api=${API_URL:-https://api.mergit.site} +gateway=${GATEWAY_URL:-https://gateway.mergit.site} +store=${STORE_URL:-https://store.mergit.site} +workspaces=${WORKSPACES:-/var/lib/mergit/workspaces} +bad=0 +ok() { echo "ok: $*"; } +fail() { echo "FAIL: $*" >&2; bad=1; } +get() { grep -E "^$1=" worker.env 2>/dev/null | tail -1 | cut -d= -f2-; } + +command -v docker >/dev/null || fail "docker is not installed" +docker info >/dev/null 2>&1 && ok "the Docker daemon answers" || fail "cannot reach the Docker daemon (is this user in the docker group?)" + +if [ -f worker.env ]; then + ok "worker.env exists" + [ "$(stat -c %a worker.env 2>/dev/null)" = 600 ] && ok "worker.env is mode 600" || fail "worker.env holds secrets: chmod 600 worker.env" + for v in WORKER_ID DATABASE_URL EVENTS_TOKEN STORE_WRITE_TOKEN WORKER_TOKEN; do + [ -n "$(get $v)" ] && ok "$v is set" || fail "$v is empty or missing in worker.env" + done + case "$(get DATABASE_URL)" in *CHANGE-ME*) fail "DATABASE_URL still has the CHANGE-ME placeholder" ;; esac + case "$(get DATABASE_URL)" in *@127.0.0.1*|*@localhost*) fail "DATABASE_URL points at this machine: use the server's private address" ;; esac + [ -n "$(get ANTHROPIC_API_KEY)$(get GROQ_API_KEY)" ] && ok "a model key is set" || fail "set ANTHROPIC_API_KEY or GROQ_API_KEY" +else + fail "worker.env is missing: copy worker.env.example and fill it in" +fi + +for f in deployments/base-sepolia.json deployments/agents-base-sepolia.json; do + if [ -f "$f" ]; then ok "$f is a file"; else + fail "$f is not a file. Copy it from mergit-contracts/deployments. If a directory of that name exists, Docker made it: remove it" + fi +done + +if [ -f deployments/agents-base-sepolia.json ] && [ -f worker.env ]; then + ids=$(python3 -c 'import json,sys; print(*[str(a["agent_id"]) for a in json.load(open(sys.argv[1]))["agents"]])' \ + deployments/agents-base-sepolia.json 2>/dev/null) || ids="" + if [ -z "$ids" ]; then fail "cannot read the agent ids from deployments/agents-base-sepolia.json"; fi + for id in $ids; do + [[ "$(get "MERGIT_AGENT_KEY_$id")" =~ ^0x[0-9a-fA-F]{64}$ ]] \ + && ok "MERGIT_AGENT_KEY_$id is a 32-byte hex key" \ + || fail "MERGIT_AGENT_KEY_$id is missing or malformed (the worker refuses to start without it)" + done +fi + +docker image inspect mergit-sandbox:dev >/dev/null 2>&1 && ok "the sandbox image exists" \ + || fail "no sandbox image: docker build -t mergit-sandbox:dev ../../control-plane/sandbox" +[ -d "$workspaces" ] && [ -w "$workspaces" ] && ok "$workspaces exists and is writable" \ + || fail "$workspaces is missing or not writable: sudo install -d -o \$USER $workspaces" +free_gb=$(df --output=avail -BG "$workspaces" 2>/dev/null | tail -1 | tr -dc 0-9) +[ "${free_gb:-0}" -ge 10 ] && ok "${free_gb} GB free for checkouts" || fail "under 10 GB free where $workspaces lives (${free_gb:-?} GB)" + +for pair in "api:$api" "gateway:$gateway" "store:$store"; do + name=${pair%%:*}; url=${pair#*:} + curl -fsS -m 8 -o /dev/null "$url/healthz" 2>/dev/null && ok "$name answers at $url/healthz" || fail "$name did not answer at $url/healthz" +done + +if docker image inspect mergit-worker:local >/dev/null 2>&1 && [ -f worker.env ]; then + docker run --rm --env-file worker.env --entrypoint python mergit-worker:local -c \ + "import os, psycopg; psycopg.connect(os.environ['DATABASE_URL'], connect_timeout=8).close()" >/dev/null 2>&1 \ + && ok "the database accepts this worker's credentials" \ + || fail "the database refused or did not answer (private address, firewall, password, CP_DB_BIND on the server?)" +else + echo "skip: the database check needs the worker image (docker compose build), run this again after it" +fi + +[ "$bad" = 0 ] && echo "ready." || { echo "not ready: fix the FAIL lines above." >&2; exit 1; } diff --git a/deploy/worker-vm/worker.env.example b/deploy/worker-vm/worker.env.example new file mode 100644 index 00000000..60668854 --- /dev/null +++ b/deploy/worker-vm/worker.env.example @@ -0,0 +1,30 @@ +# Copy to worker.env on the worker VM (chmod 600) and fill in. Never commit worker.env. +# Names only here; the values come from the server's deploy/secrets/*.env and from your model provider. + +# A fixed name, ONE PER PROCESS. The worker finds, fails and cleans up what its previous run left by this name, and +# refuses to start if another process already holds it. Never reuse a name for a second worker on the same database. +WORKER_ID=worker-vm-1 + +# Printed once by deploy/worker-vm/create-db-role.sh on the server. Use the server's PRIVATE address. +DATABASE_URL=postgres://mergit_worker:CHANGE-ME@10.0.0.2:5433/mergit_cp?sslmode=prefer + +# Same values as the server's deploy/secrets/worker.env (run `grep -E '^(EVENTS|STORE_WRITE|WORKER)_TOKEN' there`). +EVENTS_TOKEN= +STORE_WRITE_TOKEN= +WORKER_TOKEN= + +# One key per agent in agents-base-sepolia.json, 0x plus 64 hex characters. The worker refuses to start without +# every one (REQUIRE_ALL_AGENT_KEYS in compose.yaml). From mergit-contracts/.env.services. +MERGIT_AGENT_KEY_1= + +# A model key: Anthropic's, Groq's, or both. Anthropic is used when its key is present. +ANTHROPIC_API_KEY= +GROQ_API_KEY= +# GROQ_MODEL=openai/gpt-oss-120b +# LLM_PROVIDER=anthropic + +# Optional. Only the pull-request gate needs it; without it the worker still runs tasks. +# GITHUB_TOKEN= + +# Optional. gVisor, if runsc is installed on this VM. Untested here: leave unset unless you have tried it. +# SANDBOX_RUNTIME=runsc From 288c51228a20e2c2650735e8113aef846f3d0b59 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Sun, 1 Nov 2026 03:12:36 +0530 Subject: [PATCH 2/4] fix(deploy): only mergit_worker may log in to cp_postgres from outside compose The postgres:16 image's pg_hba ends with `host all all all scram-sha-256`, so once CP_DB_BIND publishes cp_postgres on the private network the worker VM can log in as the superuser with the password committed in compose.controlplane.yaml, which makes the restricted role pointless. cp_postgres now uses deploy/postgres/pg_hba.conf: any role from the compose network (samenet), and from anywhere else only mergit_worker on mergit_cp. In-container socket access (backup.sh, wallet-interest.sh, create-db-role.sh) is unchanged. --- control-plane/tests/test_cp_postgres_hba.py | 35 +++++++++++++++++++++ deploy/compose.controlplane.yaml | 5 ++- deploy/postgres/pg_hba.conf | 11 +++++++ deploy/worker-vm/README.md | 1 + 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 control-plane/tests/test_cp_postgres_hba.py create mode 100644 deploy/postgres/pg_hba.conf diff --git a/control-plane/tests/test_cp_postgres_hba.py b/control-plane/tests/test_cp_postgres_hba.py new file mode 100644 index 00000000..0da9dd33 --- /dev/null +++ b/control-plane/tests/test_cp_postgres_hba.py @@ -0,0 +1,35 @@ +"""cp_postgres must not let the superuser in from outside the compose network (deploy/postgres/pg_hba.conf). + +CP_DB_BIND publishes the port on the private network for the worker VM. With the image's default rules the +superuser would log in from there too, with the password in compose.controlplane.yaml. +""" + +import re +from pathlib import Path + +DEPLOY = Path(__file__).resolve().parents[2] / "deploy" + + +def _rules() -> list[list[str]]: + text = (DEPLOY / "postgres" / "pg_hba.conf").read_text() + return [line.split() for line in text.splitlines() if line.strip() and not line.lstrip().startswith("#")] + + +def test_cp_postgres_uses_the_repository_hba_file() -> None: + compose = (DEPLOY / "compose.controlplane.yaml").read_text() + block = re.search(r"^ cp_postgres:\n(.*?)\n\n", compose, re.S | re.M) + assert block, "no cp_postgres service" + assert "hba_file=/etc/postgresql/mergit/pg_hba.conf" in block.group(1) + assert '"./postgres:/etc/postgresql/mergit:ro"' in block.group(1) + + +def test_only_the_worker_role_may_log_in_from_outside_the_compose_network() -> None: + remote = [ + r for r in _rules() if r[0].startswith("host") and r[3] not in ("127.0.0.1/32", "::1/128", "samenet") + ] + assert remote == [["host", "mergit_cp", "mergit_worker", "all", "scram-sha-256"]] + + +def test_no_rule_trusts_a_network_address() -> None: + trusted = [r for r in _rules() if r[0].startswith("host") and r[-1] == "trust"] + assert all(r[3] in ("127.0.0.1/32", "::1/128") for r in trusted) diff --git a/deploy/compose.controlplane.yaml b/deploy/compose.controlplane.yaml index 983bfafd..82d467b6 100644 --- a/deploy/compose.controlplane.yaml +++ b/deploy/compose.controlplane.yaml @@ -14,7 +14,10 @@ services: restart: unless-stopped # The control plane does not share the gateway's database: both define tables named plans, tasks and outbox. environment: { POSTGRES_PASSWORD: mergit, POSTGRES_DB: mergit_cp } - volumes: ["cp_pgdata:/var/lib/postgresql/data"] + # deploy/postgres/pg_hba.conf: from outside this compose network, only the mergit_worker role may log in. The + # folder is mounted, not the file, so a `git pull` that replaces the file is seen (apply it with a restart). + command: ["postgres", "-c", "hba_file=/etc/postgresql/mergit/pg_hba.conf"] + volumes: ["cp_pgdata:/var/lib/postgresql/data", "./postgres:/etc/postgresql/mergit:ro"] healthcheck: { test: ["CMD", "pg_isready", "-U", "postgres"], interval: 2s, retries: 30 } # Only the worker VM connects from outside (deploy/worker-vm/README.md), as the restricted mergit_worker role. # Loopback until CP_DB_BIND (deploy/.env) names this server's private-network address. Never 0.0.0.0: Docker's diff --git a/deploy/postgres/pg_hba.conf b/deploy/postgres/pg_hba.conf new file mode 100644 index 00000000..54d2b6ff --- /dev/null +++ b/deploy/postgres/pg_hba.conf @@ -0,0 +1,11 @@ +# Who may log in to cp_postgres (deploy/compose.controlplane.yaml). It replaces the image's default, whose last line, +# `host all all all scram-sha-256`, lets the superuser in from any address that reaches the port, with the password +# in compose.controlplane.yaml. Once CP_DB_BIND publishes the port on the private network (deploy/worker-vm), that +# address would include the worker VM, the machine most likely to be compromised, and the restricted role would be +# pointless. So: the containers on this compose network may use any role, and anything else only mergit_worker. +# Docker keeps the source address of a connection to a published port, so the worker VM is never `samenet` here. +local all all trust +host all all 127.0.0.1/32 trust +host all all ::1/128 trust +host all all samenet scram-sha-256 +host mergit_cp mergit_worker all scram-sha-256 diff --git a/deploy/worker-vm/README.md b/deploy/worker-vm/README.md index 4a99f1ae..73f25f1e 100644 --- a/deploy/worker-vm/README.md +++ b/deploy/worker-vm/README.md @@ -16,6 +16,7 @@ model keys and one restricted database role. It has no gateway key, no treasury 1. A second Hetzner server in the same project, plus a private network joining both. Note the server's private address. 2. `deploy/.env` (git-ignored; compose reads it): `CP_DB_BIND=` and `WORKER_VM_IP=`. Leave `CP_DB_BIND` unset and Postgres stays on loopback. Never set it to `0.0.0.0`: Docker's published ports skip the host firewall. + Through that port only `mergit_worker` can log in (`deploy/postgres/pg_hba.conf`); the `postgres` superuser cannot. 3. Make the role (prints its URL once): `deploy/worker-vm/create-db-role.sh`. It grants only `jobs`, `pr_requests` (read and update) and `plans`, `tasks` (read). No sessions, users, wallets or email addresses. 4. `docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml -f deploy/compose.server.yaml up -d`. From ddd1c048af21b2dc654932a47938868310a8b125 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Mon, 2 Nov 2026 19:43:33 +0530 Subject: [PATCH 3/4] deploy: pin the compose subnet, bind api's port, reorder the role script, fail-fast db connects Follow-up to Viscous106's review of #38. - pg_hba.conf matched the compose network with samenet, which under some Docker network settings (docker-proxy's own bridge gateway, iptables: false) is the address a published port sees a connection arrive from: the superuser, password and all, would then be reachable from outside. The compose network now pins a subnet (networks.default.ipam) and pg_hba.conf matches that exact CIDR instead. - api's 8000:8000 published to every interface, bypassing the Caddy lockdown for a run without compose.server.yaml. Bound to 127.0.0.1. - deploy/worker-vm/README.md ran create-db-role.sh before the stack that creates the control plane's tables, which create-db-role.sh itself says it needs. Swapped the order. - The three places a worker or the api reconnects to Postgres (WorkerIdLock, PgJobStore, PgPrStore) had no connect_timeout, so a database that is down or unreachable hung the call forever instead of letting start-up or a reconnect fail and get retried. Two new tests pin the subnet and the api binding so they cannot drift back to samenet or an open port silently. Checked locally: full control-plane suite (328 passed), ruff and pyright clean, and `docker compose config` resolves correctly for compose.testnet+controlplane, and with compose.server.yaml layered on top (api's port is fully unpublished there, as before). --- control-plane/mergit_cp/pr_store.py | 8 ++++- control-plane/mergit_worker/jobs.py | 8 ++++- control-plane/mergit_worker/startup.py | 4 ++- control-plane/tests/test_cp_postgres_hba.py | 36 ++++++++++++++++++++- deploy/compose.controlplane.yaml | 12 ++++++- deploy/postgres/pg_hba.conf | 9 ++++-- deploy/worker-vm/README.md | 8 +++-- 7 files changed, 75 insertions(+), 10 deletions(-) diff --git a/control-plane/mergit_cp/pr_store.py b/control-plane/mergit_cp/pr_store.py index 96d0fdf7..78bbe456 100644 --- a/control-plane/mergit_cp/pr_store.py +++ b/control-plane/mergit_cp/pr_store.py @@ -125,9 +125,15 @@ def connect(cls, database_url: str, worker_id: str = "api") -> PgPrStore: @staticmethod def _open(database_url: str) -> psycopg.Connection[dict[str, Any]]: + # connect_timeout: a database that is down or unreachable must not hang this reconnect forever. return cast( "psycopg.Connection[dict[str, Any]]", - psycopg.connect(database_url, autocommit=True, row_factory=dict_row), # pyright: ignore[reportArgumentType] + psycopg.connect( + database_url, + autocommit=True, + row_factory=dict_row, # pyright: ignore[reportArgumentType] + connect_timeout=10, + ), ) def _retrying(self, fn: Callable[[], _T]) -> _T: diff --git a/control-plane/mergit_worker/jobs.py b/control-plane/mergit_worker/jobs.py index 3720cc22..01961be8 100644 --- a/control-plane/mergit_worker/jobs.py +++ b/control-plane/mergit_worker/jobs.py @@ -97,9 +97,15 @@ def connect(cls, database_url: str, worker_id: str) -> PgJobStore: @staticmethod def _open(database_url: str) -> psycopg.Connection[dict[str, Any]]: + # connect_timeout: a database that is down or unreachable must not hang this reconnect forever. return cast( "psycopg.Connection[dict[str, Any]]", - psycopg.connect(database_url, autocommit=True, row_factory=dict_row), # pyright: ignore[reportArgumentType] + psycopg.connect( + database_url, + autocommit=True, + row_factory=dict_row, # pyright: ignore[reportArgumentType] + connect_timeout=10, + ), ) def _retrying(self, fn: Callable[[], _T]) -> _T: diff --git a/control-plane/mergit_worker/startup.py b/control-plane/mergit_worker/startup.py index 6402f973..f2f04d5c 100644 --- a/control-plane/mergit_worker/startup.py +++ b/control-plane/mergit_worker/startup.py @@ -96,7 +96,9 @@ def __init__(self, conn: psycopg.Connection[tuple[object, ...]]) -> None: @classmethod def acquire(cls, database_url: str, worker_id: str) -> WorkerIdLock | None: - conn = psycopg.connect(database_url, autocommit=True) # pyright: ignore[reportArgumentType] + # Without a timeout, a database that is down or unreachable (a dead host, a network partition) hangs + # this call forever instead of letting start-up fail and Docker's restart policy retry it. + conn = psycopg.connect(database_url, autocommit=True, connect_timeout=10) # pyright: ignore[reportArgumentType] row = conn.execute( "SELECT pg_try_advisory_lock(hashtextextended(%s, 0))", (f"mergit_worker:{worker_id}",) ).fetchone() diff --git a/control-plane/tests/test_cp_postgres_hba.py b/control-plane/tests/test_cp_postgres_hba.py index 0da9dd33..70159521 100644 --- a/control-plane/tests/test_cp_postgres_hba.py +++ b/control-plane/tests/test_cp_postgres_hba.py @@ -15,6 +15,15 @@ def _rules() -> list[list[str]]: return [line.split() for line in text.splitlines() if line.strip() and not line.lstrip().startswith("#")] +def _compose_subnet() -> str: + """The CIDR pinned in compose.controlplane.yaml's `networks.default.ipam`, which pg_hba.conf must match + exactly: a looser or different rule there would silently stop matching this file's `all all` line.""" + compose = (DEPLOY / "compose.controlplane.yaml").read_text() + match = re.search(r"subnet:\s*(\S+)\s*}", compose) + assert match, "no pinned subnet in compose.controlplane.yaml's networks.default.ipam" + return match.group(1) + + def test_cp_postgres_uses_the_repository_hba_file() -> None: compose = (DEPLOY / "compose.controlplane.yaml").read_text() block = re.search(r"^ cp_postgres:\n(.*?)\n\n", compose, re.S | re.M) @@ -23,13 +32,38 @@ def test_cp_postgres_uses_the_repository_hba_file() -> None: assert '"./postgres:/etc/postgresql/mergit:ro"' in block.group(1) +def test_the_compose_network_is_pinned_not_left_to_docker_to_pick() -> None: + # `samenet` in pg_hba.conf would match whatever subnet Docker assigns, including a bridge gateway address + # a published port can see as its source under some Docker network settings. A fixed CIDR cannot. + subnet = _compose_subnet() + assert re.fullmatch(r"\d+\.\d+\.\d+\.\d+/\d+", subnet), subnet + + def test_only_the_worker_role_may_log_in_from_outside_the_compose_network() -> None: + compose_subnet = _compose_subnet() remote = [ - r for r in _rules() if r[0].startswith("host") and r[3] not in ("127.0.0.1/32", "::1/128", "samenet") + r + for r in _rules() + if r[0].startswith("host") and r[3] not in ("127.0.0.1/32", "::1/128", "samenet", compose_subnet) ] assert remote == [["host", "mergit_cp", "mergit_worker", "all", "scram-sha-256"]] +def test_the_compose_network_rule_uses_the_pinned_subnet_not_samenet() -> None: + compose_subnet = _compose_subnet() + broad = [r for r in _rules() if r[0:3] == ["host", "all", "all"] and r[-1] == "scram-sha-256"] + assert broad == [["host", "all", "all", compose_subnet, "scram-sha-256"]] + + def test_no_rule_trusts_a_network_address() -> None: trusted = [r for r in _rules() if r[0].startswith("host") and r[-1] == "trust"] assert all(r[3] in ("127.0.0.1/32", "::1/128") for r in trusted) + + +def test_the_api_port_is_not_published_to_every_interface() -> None: + # On the server, compose.server.yaml resets this to nothing and Caddy reaches `api` by name. Published to + # 0.0.0.0 here, a run without compose.server.yaml would expose the unproxied API to the internet. + compose = (DEPLOY / "compose.controlplane.yaml").read_text() + block = re.search(r"^ api:\n(.*?)\n\n", compose, re.S | re.M) + assert block, "no api service" + assert re.search(r'ports:\s*\["127\.0\.0\.1:8000:8000"\]', block.group(1)) diff --git a/deploy/compose.controlplane.yaml b/deploy/compose.controlplane.yaml index 82d467b6..c7909bb1 100644 --- a/deploy/compose.controlplane.yaml +++ b/deploy/compose.controlplane.yaml @@ -8,6 +8,13 @@ # The API and the worker get separate secret files so each holds only what it needs: neither has the gateway or # treasury key, and only the worker has an agent runtime key and the Docker socket. +# Pinned so deploy/postgres/pg_hba.conf can match it by an exact CIDR instead of `samenet`, which can be widened by +# how a given host's Docker handles published ports (see the comment there). Change both together. +networks: + default: + ipam: + config: [{ subnet: 172.30.9.0/24 }] + services: cp_postgres: image: postgres:16 @@ -34,7 +41,10 @@ services: image: mergit-api:local restart: unless-stopped env_file: secrets/api.env - ports: ["8000:8000"] + # Loopback only: on the server, compose.server.yaml resets this to nothing and Caddy reaches `api` by name + # instead (deploy/Caddyfile). Published to all interfaces this would be the unproxied API, open to the + # internet and skipping the Caddy lockdown, for a run without compose.server.yaml. + ports: ["127.0.0.1:8000:8000"] depends_on: cp_postgres: { condition: service_healthy } volumes: diff --git a/deploy/postgres/pg_hba.conf b/deploy/postgres/pg_hba.conf index 54d2b6ff..32d4628d 100644 --- a/deploy/postgres/pg_hba.conf +++ b/deploy/postgres/pg_hba.conf @@ -3,9 +3,14 @@ # in compose.controlplane.yaml. Once CP_DB_BIND publishes the port on the private network (deploy/worker-vm), that # address would include the worker VM, the machine most likely to be compromised, and the restricted role would be # pointless. So: the containers on this compose network may use any role, and anything else only mergit_worker. -# Docker keeps the source address of a connection to a published port, so the worker VM is never `samenet` here. +# +# The compose network is pinned to the subnet below (deploy/compose.controlplane.yaml, `networks.default.ipam`) +# and matched here by that exact CIDR, not `samenet`: `samenet` matches any address sharing a subnet with any of +# the server's interfaces, which under some Docker network settings (docker-proxy's own bridge gateway, +# `iptables: false`) is the address a published port sees a connection arrive from, superuser password and all. +# A fixed, narrow CIDR cannot widen that way. Keep the two in step if the subnet is ever changed. local all all trust host all all 127.0.0.1/32 trust host all all ::1/128 trust -host all all samenet scram-sha-256 +host all all 172.30.9.0/24 scram-sha-256 host mergit_cp mergit_worker all scram-sha-256 diff --git a/deploy/worker-vm/README.md b/deploy/worker-vm/README.md index 73f25f1e..ee2f2bb2 100644 --- a/deploy/worker-vm/README.md +++ b/deploy/worker-vm/README.md @@ -17,9 +17,11 @@ model keys and one restricted database role. It has no gateway key, no treasury 2. `deploy/.env` (git-ignored; compose reads it): `CP_DB_BIND=` and `WORKER_VM_IP=`. Leave `CP_DB_BIND` unset and Postgres stays on loopback. Never set it to `0.0.0.0`: Docker's published ports skip the host firewall. Through that port only `mergit_worker` can log in (`deploy/postgres/pg_hba.conf`); the `postgres` superuser cannot. -3. Make the role (prints its URL once): `deploy/worker-vm/create-db-role.sh`. It grants only `jobs`, `pr_requests` - (read and update) and `plans`, `tasks` (read). No sessions, users, wallets or email addresses. -4. `docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml -f deploy/compose.server.yaml up -d`. +3. `docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml -f deploy/compose.server.yaml up -d`. +4. Make the role (prints its URL once): `deploy/worker-vm/create-db-role.sh`. It needs `cp_postgres` up and the + control plane's tables already created, which is why it comes after step 3, not before. It grants only + `jobs`, `pr_requests` (read and update) and `plans`, `tasks` (read). No sessions, users, wallets or email + addresses. 5. DNS: an A record for `api` to the server (it exists today). The Hetzner firewall stays 22, 80, 443; the private network is not filtered by it. ## 2. On the worker VM From c485742b2fad0df4e772de118c4a392379a9aa5c Mon Sep 17 00:00:00 2001 From: Yash Nitin Virulkar Date: Tue, 3 Nov 2026 11:25:05 +0530 Subject: [PATCH 4/4] Back to the authors' version of this PR (merged with main); drop my additions --- control-plane/mergit_cp/health.py | 51 +-- control-plane/mergit_cp/pr_store.py | 10 +- control-plane/mergit_worker/__main__.py | 55 +-- control-plane/mergit_worker/config.py | 4 - control-plane/mergit_worker/jobs.py | 10 +- control-plane/mergit_worker/startup.py | 272 +-------------- control-plane/tests/test_cp_postgres_hba.py | 175 ++-------- control-plane/tests/test_deploy_scripts.py | 204 ----------- control-plane/tests/test_health.py | 94 ----- control-plane/tests/test_startup.py | 366 +------------------- control-plane/tests/test_worker_role.py | 301 +--------------- deploy/compose.controlplane.yaml | 51 +-- deploy/make-controlplane-env.sh | 27 +- deploy/postgres/pg_hba.conf | 30 +- deploy/server/README.md | 84 +---- deploy/server/update.sh | 33 +- deploy/worker-vm/README.md | 37 +- deploy/worker-vm/create-db-role.sh | 34 +- deploy/worker-vm/db-role.sql | 45 +-- deploy/worker-vm/preflight.sh | 13 +- deploy/worker-vm/worker.env.example | 4 - 21 files changed, 155 insertions(+), 1745 deletions(-) delete mode 100644 control-plane/tests/test_deploy_scripts.py diff --git a/control-plane/mergit_cp/health.py b/control-plane/mergit_cp/health.py index ddbdbcae..001802cf 100644 --- a/control-plane/mergit_cp/health.py +++ b/control-plane/mergit_cp/health.py @@ -6,10 +6,9 @@ import asyncio import logging -import time from typing import Annotated -from fastapi import APIRouter, Depends, Request +from fastapi import APIRouter, Depends from fastapi.responses import JSONResponse from .db import Pool @@ -21,13 +20,6 @@ # Under a proxy's own health timeout, so a stuck database answers 503 instead of hanging the check. DB_CHECK_TIMEOUT_S = 3.0 -# The route is public and unauthenticated: a flood must not become a flood of pool checkouts. One probe at a -# time is shared by every concurrent request, and its answer (ok or not) is reused for this long. -CACHE_TTL_S = 2.0 - - -def _now() -> float: - return time.monotonic() async def _database_answers(pool: Pool) -> None: @@ -35,38 +27,11 @@ async def _database_answers(pool: Pool) -> None: await conn.execute("SELECT 1") -class _Probe: - """The last database check and the one check that may be in flight.""" - - def __init__(self) -> None: - self.ok = False - self.at: float | None = None - self.task: asyncio.Task[bool] | None = None - - async def _run(self, pool: Pool) -> bool: - try: - await asyncio.wait_for(_database_answers(pool), DB_CHECK_TIMEOUT_S) - ok = True - except Exception as exc: # any failure, including a timeout or an exhausted pool, means "not healthy" - # One line, no traceback: a flood of failures must not become a flood of stack traces in the log. - log.warning("healthz: the database did not answer (%s)", type(exc).__name__) - ok = False - self.ok, self.at = ok, _now() - return ok - - async def check(self, pool: Pool) -> bool: - if self.at is not None and _now() - self.at < CACHE_TTL_S: - return self.ok - if self.task is None or self.task.done(): - self.task = asyncio.ensure_future(self._run(pool)) - return await asyncio.shield(self.task) - - @router.get("/healthz", include_in_schema=False) -async def healthz(request: Request, pool: Annotated[Pool, Depends(get_pool)]) -> JSONResponse: - probe: _Probe | None = getattr(request.app.state, "healthz_probe", None) - if probe is None: - probe = request.app.state.healthz_probe = _Probe() - if await probe.check(pool): - return JSONResponse({"ok": True}) - return JSONResponse({"ok": False}, status_code=503) +async def healthz(pool: Annotated[Pool, Depends(get_pool)]) -> JSONResponse: + try: + await asyncio.wait_for(_database_answers(pool), DB_CHECK_TIMEOUT_S) + except Exception: # any failure, including a timeout or an exhausted pool, means "not healthy" + log.warning("healthz: the database did not answer", exc_info=True) + return JSONResponse({"ok": False}, status_code=503) + return JSONResponse({"ok": True}) diff --git a/control-plane/mergit_cp/pr_store.py b/control-plane/mergit_cp/pr_store.py index 42e563fc..78bbe456 100644 --- a/control-plane/mergit_cp/pr_store.py +++ b/control-plane/mergit_cp/pr_store.py @@ -12,7 +12,6 @@ from typing import Any, TypeVar, cast import psycopg -from psycopg.conninfo import conninfo_to_dict from psycopg.rows import dict_row from .pr_gate import PlanInfo, TaskChange @@ -126,16 +125,15 @@ def connect(cls, database_url: str, worker_id: str = "api") -> PgPrStore: @staticmethod def _open(database_url: str) -> psycopg.Connection[dict[str, Any]]: + # connect_timeout: a database that is down or unreachable must not hang this reconnect forever. return cast( "psycopg.Connection[dict[str, Any]]", psycopg.connect( database_url, autocommit=True, - row_factory=dict_row, - # An unreachable database must fail in seconds, not after the kernel's TCP timeout. A - # connect_timeout in the URL itself wins. - **({} if "connect_timeout" in conninfo_to_dict(database_url) else {"connect_timeout": 10}), - ), # pyright: ignore[reportArgumentType] + row_factory=dict_row, # pyright: ignore[reportArgumentType] + connect_timeout=10, + ), ) def _retrying(self, fn: Callable[[], _T]) -> _T: diff --git a/control-plane/mergit_worker/__main__.py b/control-plane/mergit_worker/__main__.py index a7e2566a..e83121f3 100644 --- a/control-plane/mergit_worker/__main__.py +++ b/control-plane/mergit_worker/__main__.py @@ -8,8 +8,6 @@ import threading from dataclasses import dataclass -import psycopg - from mergit_cp.pr_store import PgPrStore from .agents import validate_agents_are_priced @@ -25,16 +23,7 @@ from .providers import llm_provider from .publish import ReceiptPublisher from .sandbox import Sandbox, SandboxConfig, reap_stale -from .startup import ( - KEEPALIVES_COUNT, - KEEPALIVES_IDLE_S, - KEEPALIVES_INTERVAL_S, - LOCK_CHECK_INTERVAL_S, - acquire_with_grace, - agents_without_key, - sweep_workspaces, - watch_lock, -) +from .startup import WorkerIdLock, agents_without_key, sweep_workspaces from .tools import ToolLimits from .worker import Worker, WorkerDeps from .workspace import GitLimits @@ -173,29 +162,16 @@ def main() -> int: log.warning( "no usable key for agent(s) %s: a job for them fails, and the requester loses an attempt", names ) - try: - lock = acquire_with_grace(cfg.database_url.get_secret_value(), worker_id, cfg.worker_id_lock_grace_s) - except psycopg.Error as exc: - # Exit non-zero without a traceback; Docker's restart policy is the retry loop. - log.error("cannot reach the database at start-up: %s", " ".join(str(exc).split())) - return 1 + lock = WorkerIdLock.acquire(cfg.database_url.get_secret_value(), worker_id) if lock is None: log.error( "another worker process is already running as %r. Starting here would fail its running jobs and " - "remove its sandbox containers. Give each process its own WORKER_ID. If this VM has just lost " - "power or its network, the database still holds the previous run's session until TCP keepalives " - "go unanswered (about %d s after its last packet); Docker's restart policy retries until then.", + "remove its sandbox containers. Give each process its own WORKER_ID.", worker_id, - KEEPALIVES_IDLE_S + KEEPALIVES_INTERVAL_S * KEEPALIVES_COUNT, ) return 2 log.info("model provider: %s", llm_provider()) - try: - built = build_worker(cfg, worker_id) - except psycopg.Error as exc: - log.error("cannot reach the database at start-up: %s", " ".join(str(exc).split())) - lock.close() - return 1 + built = build_worker(cfg, worker_id) log.info("failed %d jobs this worker left running before a restart", built.jobs.fail_own_running()) log.info( "reclaimed %d jobs left running past their lease by any worker", @@ -225,33 +201,12 @@ def request_stop(_signum: int, _frame: object) -> None: for sig in (signal.SIGINT, signal.SIGTERM): signal.signal(sig, request_stop) - lost = threading.Event() - - def on_lost() -> None: - # Stopping is graceful: the current job runs to its end, so its result is published after another process - # owns this id (and has already failed the job). Accepted: the compose stop_grace_period is 40 minutes and - # the job's row is guarded by its own state, so the late write is refused or harmless. - log.error( - "another process took the worker id %r while the database was unreachable, and has failed this " - "worker's running jobs. Stopping after the current job.", - worker_id, - ) - lost.set() - stop.set() - - threading.Thread( - target=watch_lock, - args=(lock, stop, LOCK_CHECK_INTERVAL_S, on_lost), - name="worker-id-lock", - daemon=True, - ).start() try: built.worker.run_forever(stop) finally: - stop.set() built.close() lock.close() - return 2 if lost.is_set() else 0 + return 0 if __name__ == "__main__": diff --git a/control-plane/mergit_worker/config.py b/control-plane/mergit_worker/config.py index 5adabe0c..487e9d63 100644 --- a/control-plane/mergit_worker/config.py +++ b/control-plane/mergit_worker/config.py @@ -66,10 +66,6 @@ class WorkerConfig(BaseSettings): # Refuse to start unless every agent in the agents file has a MERGIT_AGENT_KEY_. Off by default: a # worker may deliberately serve only some of the agents (startup.agents_without_key logs the rest). require_all_agent_keys: bool = False - # How long start-up waits for the worker-id lock when another session holds it, before exiting 2. After a - # power loss the holder is this worker's own dead session, which the database drops about a minute after - # its last packet (startup.py), so waiting lets a reboot recover without a restart loop. 0 = do not wait. - worker_id_lock_grace_s: float = Field(default=90.0, ge=0) # Opus 5.5's thinking tokens count against this; 8192 made a truncated turn too likely. model_max_tokens: int = Field(default=16_000, ge=256) poll_interval_s: float = Field(default=2.0, gt=0) diff --git a/control-plane/mergit_worker/jobs.py b/control-plane/mergit_worker/jobs.py index 57df9f7b..01961be8 100644 --- a/control-plane/mergit_worker/jobs.py +++ b/control-plane/mergit_worker/jobs.py @@ -12,7 +12,6 @@ from typing import Any, Protocol, TypeVar, cast import psycopg -from psycopg.conninfo import conninfo_to_dict from psycopg.rows import dict_row from .models import Job, RunOutcome @@ -98,16 +97,15 @@ def connect(cls, database_url: str, worker_id: str) -> PgJobStore: @staticmethod def _open(database_url: str) -> psycopg.Connection[dict[str, Any]]: + # connect_timeout: a database that is down or unreachable must not hang this reconnect forever. return cast( "psycopg.Connection[dict[str, Any]]", psycopg.connect( database_url, autocommit=True, - row_factory=dict_row, - # An unreachable database must fail in seconds, not after the kernel's TCP timeout. A - # connect_timeout in the URL itself wins. - **({} if "connect_timeout" in conninfo_to_dict(database_url) else {"connect_timeout": 10}), - ), # pyright: ignore[reportArgumentType] + row_factory=dict_row, # pyright: ignore[reportArgumentType] + connect_timeout=10, + ), ) def _retrying(self, fn: Callable[[], _T]) -> _T: diff --git a/control-plane/mergit_worker/startup.py b/control-plane/mergit_worker/startup.py index 6f1dc99c..f2f04d5c 100644 --- a/control-plane/mergit_worker/startup.py +++ b/control-plane/mergit_worker/startup.py @@ -8,15 +8,13 @@ from __future__ import annotations -import contextlib import json import logging import os import re import shutil -import threading import time -from collections.abc import Callable, Mapping +from collections.abc import Mapping from pathlib import Path import psycopg @@ -51,25 +49,14 @@ def _remove_workspace(path: Path) -> None: outside the workspace root. """ for dirpath, dirnames, _files in os.walk(path, followlinks=False): - _chmod_700(dirpath) + os.chmod(dirpath, 0o700) for name in dirnames: child = os.path.join(dirpath, name) if not os.path.islink(child): - _chmod_700(child) + os.chmod(child, 0o700) shutil.rmtree(path) -def _chmod_700(path: str) -> None: - """chmod that never follows a symlink where the platform can say so (a link swapped in after the - ``islink`` check, by a sandbox still running, must not redirect it). Linux cannot chmod a link itself, - and raises NotImplementedError for follow_symlinks=False on one; that means it was a link, so skip it.""" - try: - os.chmod(path, 0o700, follow_symlinks=False) - except NotImplementedError: - if not os.path.islink(path): - raise - - def sweep_workspaces(root: Path, older_than_s: float, *, now: float | None = None) -> int: """Remove ``ws-*`` directories under ``root`` that nothing can still be using. Returns how many. @@ -95,255 +82,30 @@ def sweep_workspaces(root: Path, older_than_s: float, *, now: float | None = Non return removed -type _Conn = psycopg.Connection[tuple[object, ...]] - -# The lock lives exactly as long as its connection, so both ends must notice a peer that vanished without -# closing it (power loss, a network partition). Left to the kernel, a TCP connection to a dead peer lingers: -# about two hours while idle, about fifteen minutes of retransmission while data is unacknowledged. -# -# Two mechanisms, because each covers a case the other does not: -# - Keepalives probe a socket that has been idle for 30 s, every 10 s, and give up after 3 misses. They -# matter on the server's side: once a worker's VM loses power nothing arrives, the server's socket goes -# idle, and the backend (and the lock) is dropped about a minute after the last packet. On this process's -# side they rarely fire, since the watcher sends ``SELECT 1`` every 10 s and the socket is never idle 30 s. -# - tcp_user_timeout aborts a connection whose sent data has gone unacknowledged for 60 s. That is the case -# here when the database host dies: the next ``SELECT 1`` (or a query in ``_take`` right after connecting) -# is never acknowledged, and the kernel fails it after a minute instead of retransmitting for fifteen. On -# the server's side it covers a worker that dies while a reply to it is in flight. -KEEPALIVES_IDLE_S = 30 -KEEPALIVES_INTERVAL_S = 10 -KEEPALIVES_COUNT = 3 -TCP_USER_TIMEOUT_MS = 60_000 -LOCK_CONNECT_OPTIONS: dict[str, int] = { - "keepalives": 1, - "keepalives_idle": KEEPALIVES_IDLE_S, - "keepalives_interval": KEEPALIVES_INTERVAL_S, - "keepalives_count": KEEPALIVES_COUNT, - "tcp_user_timeout": TCP_USER_TIMEOUT_MS, - "connect_timeout": 10, -} -# The options above set this process's socket only. The server's socket for the same connection follows the -# server's own settings, which these session-level GUCs override (Postgres ignores them on a Unix socket). -_SERVER_KEEPALIVES = ( - "SELECT set_config('tcp_keepalives_idle', %s, false)," - " set_config('tcp_keepalives_interval', %s, false)," - " set_config('tcp_keepalives_count', %s, false)," - " set_config('tcp_user_timeout', %s, false)" -) -_SERVER_KEEPALIVE_VALUES = ( - str(KEEPALIVES_IDLE_S), - str(KEEPALIVES_INTERVAL_S), - str(KEEPALIVES_COUNT), - str(TCP_USER_TIMEOUT_MS), -) -# How long one ``SELECT 1`` may take before the lock's state counts as unknown. statement_timeout cannot help -# when the query never reaches the server, and tcp_user_timeout fails it only after a minute; until then the -# check reports "unknown" every 10 s rather than stall the watcher thread for that minute. -LOCK_PROBE_TIMEOUT_S = 10.0 - - -def _close_quietly(conn: _Conn) -> None: - with contextlib.suppress(psycopg.Error): - conn.close() - - -class _Probe: - """One ``SELECT 1`` on its own daemon thread, so a socket that never answers stalls that thread only.""" - - def __init__(self, conn: _Conn) -> None: - self.conn = conn - self.error: BaseException | None = None - self.done = threading.Event() - self.warned = False - self._lock = threading.Lock() - self._close_when_done = False - threading.Thread(target=self._run, name="worker-id-lock-probe", daemon=True).start() - - def _run(self) -> None: - try: - self.conn.execute("SELECT 1") - except BaseException as exc: # handed to the caller of still_held, which decides - self.error = exc - with self._lock: - self.done.set() - close = self._close_when_done - if close: - _close_quietly(self.conn) - - def close_when_done(self) -> None: - """Close the connection now if the query has returned, otherwise as soon as it does. psycopg's - close() frees the libpq connection, which must not happen under a thread still waiting on it.""" - with self._lock: - if not self.done.is_set(): - self._close_when_done = True - return - _close_quietly(self.conn) - - class WorkerIdLock: """A session-level Postgres advisory lock named for the worker id, held while the process lives. A worker id must belong to one process. At start-up a worker fails every job still ``running`` under its id and removes every sandbox container labelled with it, so a second process sharing the id would destroy the first one's live work. Postgres releases the lock when the connection ends, whether the - process exits cleanly or is killed, so a crashed worker never blocks its own restart. A worker whose VM - lost power or its network never ends the connection; the server ends it once TCP keepalives or - tcp_user_timeout give up (``LOCK_CONNECT_OPTIONS``, ``_SERVER_KEEPALIVES``), about a minute after its - last packet, and until then its restart is refused. - - The same rule means a database restart drops the lock while this process lives on. ``still_held`` - notices that, reconnects and takes the lock again, and reports False only when another process has - taken the id in the meantime: that process has already failed this one's jobs, so this one must stop. - - ``still_held`` runs on the watcher thread and ``close`` on the main one. ``_mutex`` guards ``_conn``, - ``_probe`` and ``_closed`` (never network I/O), so a connection made after ``close`` is closed, not kept. + process exits cleanly or is killed, so a crashed worker never blocks its own restart. """ - def __init__(self, conn: _Conn, database_url: str, key: str) -> None: - self._conn: _Conn | None = conn - self._url = database_url - self._key = key - self._mutex = threading.Lock() - self._closed = False - self._probe: _Probe | None = None + def __init__(self, conn: psycopg.Connection[tuple[object, ...]]) -> None: + self._conn = conn @classmethod def acquire(cls, database_url: str, worker_id: str) -> WorkerIdLock | None: - key = f"mergit_worker:{worker_id}" - conn = _take(database_url, key) - return None if conn is None else cls(conn, database_url, key) - - def still_held(self) -> bool | None: - """True while the lock is held, False once another process owns the id, None while that cannot be - told: the database cannot be reached (nothing can have taken the id from a database that is down), - the lock is closed, or ``SELECT 1`` has not answered within ``LOCK_PROBE_TIMEOUT_S``. - - A query that has not answered is waited for again on the next call, never abandoned for a new - connection: the old session may still hold the lock, and a new one would then report it lost.""" - with self._mutex: - if self._closed: - return None - conn, probe = self._conn, self._probe - if probe is None and conn is not None and not conn.closed: - probe = self._probe = _Probe(conn) - if probe is not None: - if not probe.done.wait(LOCK_PROBE_TIMEOUT_S): - if not probe.warned: - probe.warned = True - log.warning( - "the worker-id lock's connection has not answered for %.0f s; treating the lock as " - "unknown until it does or the connection fails", - LOCK_PROBE_TIMEOUT_S, - ) - return None - with self._mutex: - if self._probe is probe: - self._probe = None - if probe.error is None: - return True - if not isinstance(probe.error, psycopg.Error): - raise probe.error - self._drop(probe.conn) - return self._retake() - - def _retake(self) -> bool | None: - try: - conn = _take(self._url, self._key) - except psycopg.Error: - return None - with self._mutex: - closed = self._closed - if not closed: - self._conn = conn - if closed: # close() ran while this was connecting: keeping the connection would keep the lock - if conn is not None: - _close_quietly(conn) - return None - if conn is None: - return False - log.warning("the worker-id lock was lost with the database connection and has been taken again") - return True - - def _drop(self, conn: _Conn) -> None: - with self._mutex: - if self._conn is conn: - self._conn = None - _close_quietly(conn) - - def close(self) -> None: - """Release the lock. Never blocks on the network: a connection a probe is still waiting on is - closed by that probe when its query returns (or by the OS when the process exits).""" - with self._mutex: - self._closed = True - conn, self._conn = self._conn, None - probe, self._probe = self._probe, None - if probe is not None and probe.conn is conn: - probe.close_when_done() - elif conn is not None: - _close_quietly(conn) - - -def _take(database_url: str, key: str) -> _Conn | None: - conn: _Conn = psycopg.connect(database_url, autocommit=True, **LOCK_CONNECT_OPTIONS) # pyright: ignore[reportArgumentType] - try: - conn.execute(_SERVER_KEEPALIVES, _SERVER_KEEPALIVE_VALUES) - row = conn.execute("SELECT pg_try_advisory_lock(hashtextextended(%s, 0))", (key,)).fetchone() - except BaseException: + # Without a timeout, a database that is down or unreachable (a dead host, a network partition) hangs + # this call forever instead of letting start-up fail and Docker's restart policy retry it. + conn = psycopg.connect(database_url, autocommit=True, connect_timeout=10) # pyright: ignore[reportArgumentType] + row = conn.execute( + "SELECT pg_try_advisory_lock(hashtextextended(%s, 0))", (f"mergit_worker:{worker_id}",) + ).fetchone() + if row is not None and row[0] is True: + return cls(conn) conn.close() - raise - if row is not None and row[0] is True: - return conn - conn.close() - return None + return None - -LOCK_CHECK_INTERVAL_S = 10.0 -LOCK_RETRY_INTERVAL_S = 5.0 - - -def acquire_with_grace( - database_url: str, - worker_id: str, - grace_s: float, - *, - sleep: Callable[[float], None] = time.sleep, - clock: Callable[[], float] = time.monotonic, -) -> WorkerIdLock | None: - """``WorkerIdLock.acquire``, retried for up to ``grace_s`` while another session holds the id. - - After a power loss or a network cut that session is this worker's own, dead one, and the database drops it - about a minute after its last packet. Waiting for that is what lets a reboot recover by itself instead of - exiting 2 into Docker's restart loop. An unreachable database raises at once, as ``acquire`` does. - """ - deadline = clock() + grace_s - waited = False - while True: - lock = WorkerIdLock.acquire(database_url, worker_id) - if lock is not None: - if waited: - log.info("the worker-id lock for %r was released; continuing", worker_id) - return lock - remaining = deadline - clock() - if remaining <= 0: - return None - if not waited: - waited = True - log.warning( - "the worker-id lock for %r is held by another database session. If this VM just lost power " - "or its network that is its own previous session, which the database drops within about " - "a minute; waiting up to %.0f s for it", - worker_id, - grace_s, - ) - sleep(min(LOCK_RETRY_INTERVAL_S, remaining)) - - -def watch_lock( - lock: WorkerIdLock, stop: threading.Event, interval_s: float, on_lost: Callable[[], None] -) -> None: - """Check the lock every ``interval_s`` until ``stop`` is set, calling ``on_lost`` once if another - process takes the worker id.""" - while not stop.wait(interval_s): - if lock.still_held() is False: - on_lost() - return + def close(self) -> None: + self._conn.close() diff --git a/control-plane/tests/test_cp_postgres_hba.py b/control-plane/tests/test_cp_postgres_hba.py index e4cd9a2d..70159521 100644 --- a/control-plane/tests/test_cp_postgres_hba.py +++ b/control-plane/tests/test_cp_postgres_hba.py @@ -1,28 +1,13 @@ """cp_postgres must not let the superuser in from outside the compose network (deploy/postgres/pg_hba.conf). CP_DB_BIND publishes the port on the private network for the worker VM. With the image's default rules the -superuser would log in from there too. A rule naming the compose network has to leave out the bridge -gateway's address: behind Docker's userland proxy (and with rootless Docker or iptables off) a connection to a -published port reaches the container FROM the gateway, so `samenet`, which includes it, would let the worker -VM in as `postgres`. +superuser would log in from there too, with the password in compose.controlplane.yaml. """ -from __future__ import annotations - -import ipaddress -import os import re -import shutil -import stat -import subprocess from pathlib import Path -import pytest - DEPLOY = Path(__file__).resolve().parents[2] / "deploy" -LOOPBACK = ("127.0.0.1/32", "::1/128") -# Addresses pg_hba.conf accepts that are not a CIDR: each one would include the bridge gateway or anything. -KEYWORDS = ("all", "samehost", "samenet") def _rules() -> list[list[str]]: @@ -30,25 +15,13 @@ def _rules() -> list[list[str]]: return [line.split() for line in text.splitlines() if line.strip() and not line.lstrip().startswith("#")] -def _compose() -> str: - """compose.controlplane.yaml without its comments (they may name what the settings must not).""" - text = (DEPLOY / "compose.controlplane.yaml").read_text() - lines = (re.sub(r"(^|\s)#.*$", "", line) for line in text.splitlines()) - return "\n".join(line for line in lines if line.strip() or not line) - - -def _network() -> dict[str, str]: - """The compose network's fixed addresses (top-level `networks:`), the one place the subnet is defined.""" - block = re.search(r"^networks:\n(.*?)(?=^\S|\Z)", _compose(), re.S | re.M) - assert block, "compose.controlplane.yaml defines no network" - plan = dict(re.findall(r"\b(subnet|ip_range|gateway): *([0-9a-f.:/]+)", block.group(1))) - assert set(plan) == {"subnet", "ip_range", "gateway"}, plan - return plan - - -def _admits_superuser(users: str) -> bool: - # `all`, the name itself, a group (+role) or a file of names (@file) may each include postgres. - return any(u in ("all", "postgres") or u[:1] in "+@" for u in users.split(",")) +def _compose_subnet() -> str: + """The CIDR pinned in compose.controlplane.yaml's `networks.default.ipam`, which pg_hba.conf must match + exactly: a looser or different rule there would silently stop matching this file's `all all` line.""" + compose = (DEPLOY / "compose.controlplane.yaml").read_text() + match = re.search(r"subnet:\s*(\S+)\s*}", compose) + assert match, "no pinned subnet in compose.controlplane.yaml's networks.default.ipam" + return match.group(1) def test_cp_postgres_uses_the_repository_hba_file() -> None: @@ -59,120 +32,38 @@ def test_cp_postgres_uses_the_repository_hba_file() -> None: assert '"./postgres:/etc/postgresql/mergit:ro"' in block.group(1) -def test_the_database_port_is_published_on_loopback_unless_cp_db_bind_says_otherwise() -> None: - """An unset CP_DB_BIND must never publish Postgres on every interface (Docker skips the host - firewall), and the API's own port must stay on loopback too, so the Caddy lockdown cannot be bypassed.""" - compose = _compose() - assert '"${CP_DB_BIND:-127.0.0.1}:5433:5432"' in compose - assert '"127.0.0.1:8000:8000"' in compose - assert not re.search(r'"(0\.0\.0\.0:)?8000:8000"', compose) - - -def test_the_compose_network_keeps_the_gateway_out_of_the_container_range() -> None: - net = _network() - subnet = ipaddress.ip_network(net["subnet"]) - containers = ipaddress.ip_network(net["ip_range"]) - gateway = ipaddress.ip_address(net["gateway"]) - assert containers.subnet_of(subnet) # pyright: ignore[reportArgumentType] - assert gateway in subnet - assert gateway not in containers - - -def test_no_rule_admits_the_postgres_superuser_from_a_subnet_the_bridge_gateway_is_in() -> None: - net = _network() - gateway = ipaddress.ip_address(net["gateway"]) - superuser = [r for r in _rules() if r[0].startswith("host") and _admits_superuser(r[2])] - addresses = [r[3] for r in superuser] - for address in addresses: - assert address not in KEYWORDS, f"{address} includes the bridge gateway" - assert "/" in address, f"{address}: a host name or a separate netmask column; use a CIDR" - assert gateway not in ipaddress.ip_network(address), f"{address} includes the gateway {gateway}" - # Loopback inside the container, and the compose network's container range from compose.controlplane.yaml. - assert sorted(addresses) == sorted([*LOOPBACK, net["ip_range"]]) +def test_the_compose_network_is_pinned_not_left_to_docker_to_pick() -> None: + # `samenet` in pg_hba.conf would match whatever subnet Docker assigns, including a bridge gateway address + # a published port can see as its source under some Docker network settings. A fixed CIDR cannot. + subnet = _compose_subnet() + assert re.fullmatch(r"\d+\.\d+\.\d+\.\d+/\d+", subnet), subnet def test_only_the_worker_role_may_log_in_from_outside_the_compose_network() -> None: - inside = (*LOOPBACK, _network()["ip_range"]) - remote = [r for r in _rules() if r[0].startswith("host") and r[3] not in inside] + compose_subnet = _compose_subnet() + remote = [ + r + for r in _rules() + if r[0].startswith("host") and r[3] not in ("127.0.0.1/32", "::1/128", "samenet", compose_subnet) + ] assert remote == [["host", "mergit_cp", "mergit_worker", "all", "scram-sha-256"]] +def test_the_compose_network_rule_uses_the_pinned_subnet_not_samenet() -> None: + compose_subnet = _compose_subnet() + broad = [r for r in _rules() if r[0:3] == ["host", "all", "all"] and r[-1] == "scram-sha-256"] + assert broad == [["host", "all", "all", compose_subnet, "scram-sha-256"]] + + def test_no_rule_trusts_a_network_address() -> None: trusted = [r for r in _rules() if r[0].startswith("host") and r[-1] == "trust"] - assert all(r[3] in LOOPBACK for r in trusted) + assert all(r[3] in ("127.0.0.1/32", "::1/128") for r in trusted) -def test_compose_has_no_literal_superuser_password() -> None: - """The password comes from secrets/cp_postgres.env, api.env and worker.env (make-controlplane-env.sh).""" - text = _compose() - assert not re.search(r"postgres(ql)?://[^\s/:@]+:[^\s@]+@", text), "a database URL with a password in it" - # Set in compose, either would override the generated value from the service's env_file. - assert "POSTGRES_PASSWORD" not in text - assert "DATABASE_URL" not in text - block = re.search(r"^ cp_postgres:\n(.*?)\n\n", text, re.S | re.M) - assert block, "no cp_postgres service" - assert re.search(r"^ env_file: secrets/cp_postgres\.env$", block.group(1), re.M) - - -# -- make-controlplane-env.sh generates the password --------------------------------------------------------- - - -def _env(path: Path) -> dict[str, str]: - lines = [line for line in path.read_text().splitlines() if line and not line.startswith("#")] - return dict(line.split("=", 1) for line in lines) - - -@pytest.mark.skipif(shutil.which("bash") is None, reason="needs bash") -def test_the_env_script_gives_postgres_the_api_and_the_worker_one_generated_password(tmp_path: Path) -> None: - # The script's layout: deploy/secrets/testnet.env, and ../../mergit-contracts/.env.services next to it. - deploy = tmp_path / "mergit" / "deploy" - (deploy / "secrets").mkdir(parents=True) - shutil.copy(DEPLOY / "make-controlplane-env.sh", deploy) - tokens = ("EVENTS_TOKEN", "CHAIN_EVENTS_TOKEN", "WORKER_TOKEN", "STORE_WRITE_TOKEN") - (deploy / "secrets" / "testnet.env").write_text("".join(f"{t}=test-{t.lower()}\n" for t in tokens)) - (tmp_path / "mergit-contracts").mkdir() - (tmp_path / "mergit-contracts" / ".env.services").write_text("MERGIT_AGENT_KEY_1=0x" + "1" * 64 + "\n") - env = {"PATH": os.environ["PATH"], "ANTHROPIC_API_KEY": "test-anthropic-key"} - - run = subprocess.run( - ["bash", str(deploy / "make-controlplane-env.sh"), "localhost:5173"], - env=env, - capture_output=True, - text=True, - check=False, - input="\n", # Enter at the GROQ_API_KEY prompt: skip it - ) - assert run.returncode == 0, run.stderr - - secrets = deploy / "secrets" - password = _env(secrets / "cp_postgres.env")["POSTGRES_PASSWORD"] - assert re.fullmatch(r"[0-9a-f]{48}", password) - url = f"postgres://postgres:{password}@cp_postgres:5432/mergit_cp" - assert _env(secrets / "api.env")["DATABASE_URL"] == url - assert _env(secrets / "worker.env")["DATABASE_URL"] == url - assert password not in run.stdout + run.stderr # names only, never values - for name in ("cp_postgres.env", "api.env", "worker.env"): - assert stat.S_IMODE((secrets / name).stat().st_mode) == 0o600, name - - -@pytest.mark.skipif(shutil.which("bash") is None, reason="needs bash") -def test_the_env_script_refusal_warns_that_regenerating_locks_out_an_existing_database( - tmp_path: Path, -) -> None: - deploy = tmp_path / "mergit" / "deploy" - (deploy / "secrets").mkdir(parents=True) - shutil.copy(DEPLOY / "make-controlplane-env.sh", deploy) - (deploy / "secrets" / "testnet.env").write_text("") - (tmp_path / "mergit-contracts").mkdir() - (tmp_path / "mergit-contracts" / ".env.services").write_text("") - (deploy / "secrets" / "api.env").write_text("X=1\n") - run = subprocess.run( - ["bash", str(deploy / "make-controlplane-env.sh")], - env={"PATH": os.environ["PATH"]}, - capture_output=True, - text=True, - check=False, - ) - assert run.returncode != 0 - assert "cp_pgdata" in run.stderr and "locked out" in run.stderr - assert "deploy/server/README.md" in run.stderr +def test_the_api_port_is_not_published_to_every_interface() -> None: + # On the server, compose.server.yaml resets this to nothing and Caddy reaches `api` by name. Published to + # 0.0.0.0 here, a run without compose.server.yaml would expose the unproxied API to the internet. + compose = (DEPLOY / "compose.controlplane.yaml").read_text() + block = re.search(r"^ api:\n(.*?)\n\n", compose, re.S | re.M) + assert block, "no api service" + assert re.search(r'ports:\s*\["127\.0\.0\.1:8000:8000"\]', block.group(1)) diff --git a/control-plane/tests/test_deploy_scripts.py b/control-plane/tests/test_deploy_scripts.py deleted file mode 100644 index 7ee4bd42..00000000 --- a/control-plane/tests/test_deploy_scripts.py +++ /dev/null @@ -1,204 +0,0 @@ -"""The deploy shell scripts, run with stub `docker`, `curl`, `git` and `sleep` on PATH (no Docker or network). - -Postgres-backed behaviour of create-db-role.sh is in tests/test_worker_role.py. -""" - -from __future__ import annotations - -import os -import shutil -import stat -import subprocess -from pathlib import Path - -import pytest - -DEPLOY = Path(__file__).resolve().parents[2] / "deploy" -SCRIPTS = ( - "server/update.sh", - "worker-vm/preflight.sh", - "worker-vm/create-db-role.sh", - "make-controlplane-env.sh", -) - -pytestmark = pytest.mark.skipif(shutil.which("bash") is None, reason="needs bash") - - -def _stub(bin_dir: Path, name: str, body: str) -> None: - path = bin_dir / name - path.write_text("#!/usr/bin/env bash\n" + body + "\n") - path.chmod(path.stat().st_mode | stat.S_IXUSR) - - -def _run(cmd: list[str], bin_dir: Path, cwd: Path, **env: str) -> subprocess.CompletedProcess[str]: - base = {"PATH": f"{bin_dir}:{os.environ['PATH']}", "HOME": str(cwd)} - return subprocess.run(cmd, env={**base, **env}, cwd=cwd, capture_output=True, text=True, check=False) - - -@pytest.mark.parametrize("script", SCRIPTS) -def test_the_script_parses(script: str) -> None: - run = subprocess.run(["bash", "-n", str(DEPLOY / script)], capture_output=True, text=True, check=False) - assert run.returncode == 0, run.stderr - - -# -- update.sh ------------------------------------------------------------------------------------------- - - -def _update_tree( - tmp_path: Path, *, api_up: bool, public_up: bool, not_running: tuple[str, ...] = () -) -> tuple[Path, Path]: - root = tmp_path / "mergit" - (root / "deploy" / "server").mkdir(parents=True) - shutil.copy(DEPLOY / "server" / "update.sh", root / "deploy" / "server") - bin_dir = tmp_path / "bin" - bin_dir.mkdir() - _stub(bin_dir, "git", "exit 0") - _stub(bin_dir, "sleep", "exit 0") - _stub(bin_dir, "curl", "exit 0" if public_up else "exit 6") - # `exec -T api` is the check over the compose network; `ps --status running --services` lists what runs. - running = [s for s in ("gateway", "store", "api", "caddy") if s not in not_running] - _stub( - bin_dir, - "docker", - 'case " $* " in\n' - f' *" exec -T api "*) exit {0 if api_up else 1} ;;\n' - f' *" --services "*) printf "%s\\n" {" ".join(running)}; exit 0 ;;\n' - "esac; exit 0", - ) - return root, bin_dir - - -def test_update_succeeds_with_a_warning_when_the_public_urls_are_not_reachable_yet(tmp_path: Path) -> None: - root, bin_dir = _update_tree(tmp_path, api_up=True, public_up=False) - run = _run(["bash", str(root / "deploy/server/update.sh"), "services"], bin_dir, root) - assert run.returncode == 0, run.stderr - assert "warning" in run.stderr and "api.mergit.site" in run.stderr - assert "api (compose network): ok" in run.stdout - - -def test_update_fails_when_the_api_never_answers_over_the_compose_network(tmp_path: Path) -> None: - root, bin_dir = _update_tree(tmp_path, api_up=False, public_up=True) - run = _run(["bash", str(root / "deploy/server/update.sh"), "services"], bin_dir, root) - assert run.returncode != 0 - assert "api container did not answer" in run.stderr - - -@pytest.mark.parametrize("service", ["gateway", "store"]) -def test_update_fails_when_the_gateway_or_store_container_is_not_running( - tmp_path: Path, service: str -) -> None: - root, bin_dir = _update_tree(tmp_path, api_up=True, public_up=True, not_running=(service,)) - run = _run(["bash", str(root / "deploy/server/update.sh"), "services"], bin_dir, root) - assert run.returncode != 0 - assert f"the {service} container is not running" in run.stderr - - -def test_update_is_not_fooled_by_a_long_service_list_that_sigpipes_grep(tmp_path: Path) -> None: - """`ps --services | grep -q` would quit at `gateway` while ps still writes: SIGPIPE, exit 141, a healthy - service reported as not running. The stub writes far more than a pipe buffer holds after the names.""" - root, bin_dir = _update_tree(tmp_path, api_up=True, public_up=True) - _stub( - bin_dir, - "docker", - 'case " $* " in\n' - ' *" exec -T api "*) exit 0 ;;\n' - ' *" --services "*) echo gateway; echo store; exec seq 1 300000 ;;\n' - "esac; exit 0", - ) - run = _run(["bash", str(root / "deploy/server/update.sh"), "services"], bin_dir, root) - assert run.returncode == 0, run.stderr - - -def test_update_fails_loudly_when_compose_ps_itself_fails(tmp_path: Path) -> None: - root, bin_dir = _update_tree(tmp_path, api_up=True, public_up=True) - _stub( - bin_dir, - "docker", - 'case " $* " in\n *" exec -T api "*) exit 0 ;;\n *" --services "*) exit 1 ;;\nesac; exit 0', - ) - run = _run(["bash", str(root / "deploy/server/update.sh"), "services"], bin_dir, root) - assert run.returncode != 0 and "container is not running" in run.stderr - - -def test_update_fails_when_the_build_fails(tmp_path: Path) -> None: - root, bin_dir = _update_tree(tmp_path, api_up=True, public_up=True) - _stub(bin_dir, "docker", 'case " $* " in *" up -d "*) exit 1 ;; esac; exit 0') - run = _run(["bash", str(root / "deploy/server/update.sh"), "services"], bin_dir, root) - assert run.returncode != 0 - - -# -- preflight.sh ---------------------------------------------------------------------------------------- - - -def _preflight_tree(tmp_path: Path, worker_env: str) -> tuple[Path, Path, Path]: - vm = tmp_path / "mergit" / "deploy" / "worker-vm" - vm.mkdir(parents=True) - shutil.copy(DEPLOY / "worker-vm" / "preflight.sh", vm) - (vm / "worker.env").write_text(worker_env) - (vm / "worker.env").chmod(0o600) - workspaces = tmp_path / "workspaces" - workspaces.mkdir() - bin_dir = tmp_path / "bin" - bin_dir.mkdir() - seen = tmp_path / "docker-run-env" - _stub(bin_dir, "python3", "exit 1") # the agent-key check is not under test - _stub(bin_dir, "curl", "exit 0") - # `docker run` records the DATABASE_URL it was given through the environment, and every arg. - _stub( - bin_dir, - "docker", - f'case "$1" in run) printf "%s" "$DATABASE_URL" > {seen}; ' - f'printf "%s\\n" "$@" > {seen}.args ;; esac; exit 0', - ) - return vm, bin_dir, workspaces - - -ENV_QUOTED = """WORKER_ID="vm-1" -DATABASE_URL='postgres://mergit_worker:pw@10.0.0.2:5433/mergit_cp?sslmode=prefer' -EVENTS_TOKEN="e" -STORE_WRITE_TOKEN='s' -WORKER_TOKEN="w" -ANTHROPIC_API_KEY="k" -""" - - -def test_preflight_strips_quotes_like_compose_and_passes_the_clean_url_to_the_database_check( - tmp_path: Path, -) -> None: - vm, bin_dir, workspaces = _preflight_tree(tmp_path, ENV_QUOTED) - run = _run(["bash", str(vm / "preflight.sh")], bin_dir, vm, WORKSPACES=str(workspaces)) - out = run.stdout + run.stderr - assert "DATABASE_URL points at this machine" not in out - assert "WORKER_ID is empty" not in out and "ok: WORKER_ID is set" in out - seen = tmp_path / "docker-run-env" - assert seen.read_text() == "postgres://mergit_worker:pw@10.0.0.2:5433/mergit_cp?sslmode=prefer" - args = (tmp_path / "docker-run-env.args").read_text() - assert "--env-file" not in args # the env file would keep the quote characters - assert "pw@" not in args # the URL is never on a command line - assert "ok: the database accepts this worker's credentials" in out - - -# -- create-db-role.sh ----------------------------------------------------------------------------------- - - -@pytest.mark.parametrize("name", ["postgres", "pg_monitor", "pg_write_all_data"]) -def test_create_db_role_refuses_the_superuser_and_builtin_names_before_running_anything( - tmp_path: Path, name: str -) -> None: - marker = tmp_path / "psql-was-run" - env = { - "PATH": os.environ["PATH"], - "PSQL": f"touch {marker}", - "WORKER_DB_ROLE": name, - "WORKER_DB_PASSWORD": "a-long-enough-password-0123456789", - } - run = subprocess.run( - ["bash", str(DEPLOY / "worker-vm/create-db-role.sh")], - env=env, - capture_output=True, - text=True, - check=False, - ) - assert run.returncode != 0 - assert "refused" in run.stderr - assert not marker.exists() and run.stdout == "" diff --git a/control-plane/tests/test_health.py b/control-plane/tests/test_health.py index d347f804..c1535e8a 100644 --- a/control-plane/tests/test_health.py +++ b/control-plane/tests/test_health.py @@ -83,97 +83,3 @@ async def connection(self) -> AsyncIterator[Conn]: asyncio.run(health._database_answers(cast(Any, Pool()))) assert seen == ["SELECT 1"] - - -class _CountingPool: - """A pool that records every checkout, how many are open at once, and can fail or hang.""" - - def __init__(self, fail: bool = False, hang: bool = False) -> None: - self.fail, self.hang = fail, hang - self.checkouts = 0 - self.open_now = 0 - self.max_open = 0 - - @asynccontextmanager - async def connection(self) -> AsyncIterator[_CountingPool]: - self.checkouts += 1 - self.open_now += 1 - self.max_open = max(self.max_open, self.open_now) - try: - yield self - finally: - self.open_now -= 1 - - async def execute(self, _sql: str) -> None: - if self.hang: - await asyncio.sleep(60) - if self.fail: - raise OSError("connection refused: password=hunter2") - - -def test_healthz_serves_a_cached_result_within_two_seconds(monkeypatch: pytest.MonkeyPatch) -> None: - now = [1000.0] - monkeypatch.setattr(health, "_now", lambda: now[0]) - pool = _CountingPool() - with TestClient(_app_with(pool)) as c: - assert c.get("/healthz").json() == {"ok": True} - now[0] += 1.9 - assert c.get("/healthz").json() == {"ok": True} - assert pool.checkouts == 1, "two calls inside 2 s must cost one database probe" - now[0] += 0.2 # past the 2 s window: probe again - assert c.get("/healthz").status_code == 200 - assert pool.checkouts == 2 - - -def test_healthz_caches_a_failure_too(monkeypatch: pytest.MonkeyPatch) -> None: - now = [1000.0] - monkeypatch.setattr(health, "_now", lambda: now[0]) - pool = _CountingPool(fail=True) - with TestClient(_app_with(pool)) as c: - assert c.get("/healthz").status_code == 503 - assert c.get("/healthz").status_code == 503 - assert pool.checkouts == 1 - - -def test_concurrent_healthz_requests_share_one_probe() -> None: - pool = _CountingPool() - - async def run() -> list[int]: - app = _app_with(pool) - from httpx import ASGITransport, AsyncClient - - async with AsyncClient(transport=ASGITransport(app=app), base_url="http://t") as c: - responses = await asyncio.gather(*(c.get("/healthz") for _ in range(20))) - return [r.status_code for r in responses] - - assert asyncio.run(run()) == [200] * 20 - assert pool.checkouts == 1 - - -def test_healthz_failure_logs_without_a_traceback_flood(caplog: pytest.LogCaptureFixture) -> None: - pool = _CountingPool(fail=True) - with caplog.at_level("DEBUG", logger="mergit_cp.health"), TestClient(_app_with(pool)) as c: - response = c.get("/healthz") - assert response.status_code == 503 - assert response.json() == {"ok": False} - records = [r for r in caplog.records if r.name == "mergit_cp.health"] - assert records, "a failed probe must be logged" - assert all(r.levelname == "WARNING" for r in records) - assert all(r.exc_info is None for r in records), "no traceback per failure" - assert "hunter2" not in response.text - - -def test_a_probe_that_hangs_returns_not_ok_and_holds_one_connection(monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr(health, "DB_CHECK_TIMEOUT_S", 0.2) - pool = _CountingPool(hang=True) - - async def run() -> list[int]: - from httpx import ASGITransport, AsyncClient - - async with AsyncClient(transport=ASGITransport(app=_app_with(pool)), base_url="http://t") as c: - responses = await asyncio.gather(*(c.get("/healthz") for _ in range(10))) - return [r.status_code for r in responses] - - assert asyncio.run(run()) == [503] * 10 - assert pool.max_open == 1 - assert pool.open_now == 0, "the timed-out probe must give its connection back" diff --git a/control-plane/tests/test_startup.py b/control-plane/tests/test_startup.py index f9d03298..23482e85 100644 --- a/control-plane/tests/test_startup.py +++ b/control-plane/tests/test_startup.py @@ -2,21 +2,14 @@ from __future__ import annotations -import contextlib import json import os -import threading -import time -from collections.abc import Callable from pathlib import Path -import psycopg import pytest -from psycopg.conninfo import conninfo_to_dict, make_conninfo -import mergit_worker.startup as startup from mergit_worker.config import ConfigError -from mergit_worker.startup import WorkerIdLock, agents_without_key, sweep_workspaces, watch_lock +from mergit_worker.startup import agents_without_key, sweep_workspaces KEY_A = "0x" + "a" * 64 KEY_B = "0x" + "b" * 64 @@ -187,360 +180,3 @@ def flaky(path: Path) -> None: assert sweep_workspaces(tmp_path, HOUR, now=now) == 1 assert stuck.exists() and not other.exists() assert "ws-a-stuck" in caplog.text - - -# -- WorkerIdLock without a database (the real-Postgres tests are in test_worker_role.py) ----------------- - - -class FakeCursor: - def __init__(self, row: tuple[object, ...] | None) -> None: - self.row = row - - def fetchone(self) -> tuple[object, ...] | None: - return self.row - - -class FakeConn: - """Enough of a psycopg connection for WorkerIdLock. ``hang`` makes ``SELECT 1`` block until it is set, - like a socket whose peer lost power; ``fail`` makes it raise once it returns.""" - - def __init__(self, *, locked: bool = True) -> None: - self.locked = locked - self.closed = False - self.sql: list[str] = [] - self.params: list[object] = [] - self.hang: threading.Event | None = None - self.fail = False - - def execute(self, sql: str, params: object = None) -> FakeCursor: - self.sql.append(sql) - self.params.append(params) - if sql == "SELECT 1": - if self.hang is not None: - self.hang.wait(3.0) # bounded, so a regression fails the test instead of hanging the run - if self.fail: - raise psycopg.OperationalError("server closed the connection unexpectedly") - return FakeCursor((1,)) - if "pg_try_advisory_lock" in sql: - return FakeCursor((self.locked,)) - return FakeCursor(None) - - def close(self) -> None: - self.closed = True - - -def test_the_lock_conninfo_sets_tcp_keepalives_and_a_connect_timeout(monkeypatch: pytest.MonkeyPatch) -> None: - """Without these a peer that lost power or dropped off the network is noticed only when the kernel gives - up: about two hours idle, or about fifteen minutes with a query unacknowledged. tcp_user_timeout is the - one that matters for a dead database host, because the watcher's SELECT 1 every 10 s keeps the socket - from ever being idle long enough for a keepalive. Without a connect timeout a reconnect can wait as - long.""" - seen: dict[str, str] = {} - conn = FakeConn() - - def connect(conninfo: str = "", *, autocommit: bool = False, **kwargs: object) -> FakeConn: - assert autocommit is True - seen.update(conninfo_to_dict(make_conninfo(conninfo, **kwargs))) # what psycopg.connect would merge - return conn - - monkeypatch.setattr(startup.psycopg, "connect", connect) - lock = WorkerIdLock.acquire("postgresql://worker@db.invalid:5433/mergit_cp", "conninfo-id") - assert lock is not None - assert { - k: seen.get(k) for k in ("keepalives", "keepalives_idle", "keepalives_interval", "keepalives_count") - } == { - "keepalives": "1", - "keepalives_idle": "30", - "keepalives_interval": "10", - "keepalives_count": "3", - } - assert seen.get("tcp_user_timeout") == "60000" # milliseconds - assert seen.get("connect_timeout") == "10" - assert seen.get("host") == "db.invalid" and seen.get("dbname") == "mergit_cp" # the URL is kept - - -def test_the_lock_session_asks_the_server_to_drop_a_client_that_went_silent( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """The client's keepalives cannot free the lock: it is the server that holds a powered-off worker's - backend, and with it the lock, until its own keepalive gives up. The session sets the server's side.""" - conn = FakeConn() - monkeypatch.setattr(startup.psycopg, "connect", lambda *_a, **_k: conn) # pyright: ignore[reportUnknownLambdaType] - assert WorkerIdLock.acquire("postgresql://worker@db.invalid/mergit_cp", "server-side-id") is not None - at = next(i for i, s in enumerate(conn.sql) if "tcp_keepalives_idle" in s) - assert conn.sql[at] == ( - "SELECT set_config('tcp_keepalives_idle', %s, false)," - " set_config('tcp_keepalives_interval', %s, false)," - " set_config('tcp_keepalives_count', %s, false)," - " set_config('tcp_user_timeout', %s, false)" - ) - assert conn.params[at] == ("30", "10", "3", "60000") - assert at < next(i for i, s in enumerate(conn.sql) if "pg_try_advisory_lock" in s) # set before locking - - -def test_a_hung_select_one_does_not_wedge_the_watcher(monkeypatch: pytest.MonkeyPatch) -> None: - """A half-open socket never answers ``SELECT 1``. The check must give up and report "unknown", not block - the watcher thread, and must not reconnect while the old session may still hold the lock.""" - monkeypatch.setattr(startup, "LOCK_PROBE_TIMEOUT_S", 0.2) - takes: list[str] = [] - monkeypatch.setattr(startup, "_take", lambda _url, key: takes.append(key)) # pyright: ignore[reportUnknownLambdaType] - conn = FakeConn() - conn.hang = threading.Event() - lock = WorkerIdLock(conn, "postgresql://unused", "mergit_worker:hung") # type: ignore[arg-type] - try: - started = time.monotonic() - assert lock.still_held() is None - assert time.monotonic() - started < 1.5 - assert lock.still_held() is None # still unknown while the same probe hangs - assert takes == [] - - lost: list[int] = [] - stop = threading.Event() - watcher = threading.Thread(target=watch_lock, args=(lock, stop, 0.01, lambda: lost.append(1))) - watcher.start() - time.sleep(0.5) - stop.set() - watcher.join(2.0) - assert not watcher.is_alive() # it kept looping and saw the stop - assert lost == [] - assert conn.sql.count("SELECT 1") == 1 # no pile of probe threads behind the hung one - finally: - conn.hang.set() - lock.close() - - -def test_a_probe_that_fails_after_hanging_reconnects_on_the_next_check( - monkeypatch: pytest.MonkeyPatch, -) -> None: - monkeypatch.setattr(startup, "LOCK_PROBE_TIMEOUT_S", 0.2) - fresh = FakeConn() - monkeypatch.setattr(startup, "_take", lambda _url, _key: fresh) # pyright: ignore[reportUnknownLambdaType] - conn = FakeConn() - conn.hang = threading.Event() - conn.fail = True - lock = WorkerIdLock(conn, "postgresql://unused", "mergit_worker:recovers") # type: ignore[arg-type] - assert lock.still_held() is None - conn.hang.set() # the kernel finally reports the socket dead - deadline = time.monotonic() + 2.0 - while not conn.closed and time.monotonic() < deadline: - assert lock.still_held() in (None, True) - time.sleep(0.01) - assert conn.closed - assert lock.still_held() is True - assert lock._conn is fresh # pyright: ignore[reportPrivateUsage] - lock.close() - assert fresh.closed - - -def test_close_while_the_watcher_is_reconnecting_leaks_no_connection(monkeypatch: pytest.MonkeyPatch) -> None: - """The process shuts down while the watcher is between connecting and storing the new connection. - Stored after close(), it would hold the lock (and a server backend) until the process exits.""" - entered, release = threading.Event(), threading.Event() - fresh = FakeConn() - - def slow_take(_url: str, _key: str) -> FakeConn: - entered.set() - release.wait(3.0) - return fresh - - monkeypatch.setattr(startup, "_take", slow_take) - broken = FakeConn() - broken.fail = True - lock = WorkerIdLock(broken, "postgresql://unused", "mergit_worker:closing") # type: ignore[arg-type] - result: list[bool | None] = [] - watcher = threading.Thread(target=lambda: result.append(lock.still_held())) - watcher.start() - assert entered.wait(2.0) - lock.close() - release.set() - watcher.join(2.0) - assert not watcher.is_alive() - assert broken.closed - assert fresh.closed - assert lock._conn is None # pyright: ignore[reportPrivateUsage] - assert result == [None] - - -def test_close_during_a_hung_probe_closes_the_connection_once_the_probe_returns( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """close() must not block on a hung socket (it runs in the shutdown path), and must not tear the - connection down under a thread that is still inside it; the probe closes it on its way out.""" - monkeypatch.setattr(startup, "LOCK_PROBE_TIMEOUT_S", 0.2) - conn = FakeConn() - conn.hang = threading.Event() - lock = WorkerIdLock(conn, "postgresql://unused", "mergit_worker:shutdown") # type: ignore[arg-type] - assert lock.still_held() is None - started = time.monotonic() - lock.close() - assert time.monotonic() - started < 0.5 - assert not conn.closed - conn.hang.set() - deadline = time.monotonic() + 2.0 - while not conn.closed and time.monotonic() < deadline: - time.sleep(0.01) - assert conn.closed - assert lock.still_held() is None # a closed lock is never "held" again, and never reconnects - - -def test_a_refused_start_says_the_previous_session_may_still_be_clearing( - monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture -) -> None: - """After a power loss the "other process" is this worker's own dead session. The operator is told it - clears by itself, so the exit-2 restarts that follow for about a minute are not read as a duplicate id.""" - from types import SimpleNamespace - - from pydantic import SecretStr - - import mergit_worker.__main__ as entry - - cfg = SimpleNamespace( - worker_id="held-id", - agents_file=Path("unused"), - require_all_agent_keys=False, - worker_id_lock_grace_s=0.0, - database_url=SecretStr("postgresql://nobody@db.invalid/none"), - ) - monkeypatch.setattr(entry, "load_config", lambda: cfg) - monkeypatch.setattr(entry, "agents_without_key", lambda _path: []) # pyright: ignore[reportUnknownLambdaType] - monkeypatch.setattr(startup.WorkerIdLock, "acquire", classmethod(lambda _cls, _url, _id: None)) # pyright: ignore[reportUnknownLambdaType, reportUnknownArgumentType] - with caplog.at_level("ERROR"): - assert entry.main() == 2 - assert "already running as 'held-id'" in caplog.text - assert "about 60 s after its last packet" in caplog.text - - -def test_startup_connect_has_a_connect_timeout(monkeypatch: pytest.MonkeyPatch) -> None: - """Every connection the worker process opens at start-up (the id lock, the job store, the pull-request - store) must give up on an unreachable database instead of waiting on the kernel's TCP timeout. The two - stores let a timeout set in the URL itself win; the lock's options are fixed.""" - import mergit_cp.pr_store as pr_store_module - import mergit_worker.jobs as jobs_module - - def seen_by(call: Callable[[str], object], url: str) -> dict[str, str]: - seen: dict[str, str] = {} - - def connect( - conninfo: str = "", *, autocommit: bool = False, row_factory: object = None, **kwargs: object - ) -> FakeConn: - seen.update(conninfo_to_dict(make_conninfo(conninfo, **kwargs))) - return FakeConn() - - monkeypatch.setattr(startup.psycopg, "connect", connect) - with contextlib.suppress(Exception): # the fake is not a real connection; only the options matter - call(url) - return seen - - url = "postgresql://worker@db.invalid:5433/mergit_cp" - assert seen_by(lambda u: WorkerIdLock.acquire(u, "timeout-id"), url).get("connect_timeout") == "10" - for name, open_ in ( - ("jobs", jobs_module.PgJobStore._open), - ("pr_store", pr_store_module.PgPrStore._open), - ): - assert seen_by(open_, url).get("connect_timeout") == "10", name - assert seen_by(open_, url + "?connect_timeout=3").get("connect_timeout") == "3", name - - -# -- waiting for the worker-id lock at start-up --------------------------------------------------------- - - -class _Clock: - """A fake clock whose sleep advances it, so the grace wait runs in no real time.""" - - def __init__(self) -> None: - self.now = 0.0 - self.sleeps: list[float] = [] - - def sleep(self, seconds: float) -> None: - self.sleeps.append(seconds) - self.now += seconds - - -def _acquire_results(monkeypatch: pytest.MonkeyPatch, results: list[object]) -> list[int]: - calls: list[int] = [] - - def acquire(_cls: object, _url: str, _id: str) -> object: - calls.append(1) - return results.pop(0) if results else None - - monkeypatch.setattr(startup.WorkerIdLock, "acquire", classmethod(acquire)) # pyright: ignore[reportArgumentType] - return calls - - -def test_a_held_lock_is_waited_for_and_taken_once_it_frees( - monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture -) -> None: - """After a reboot the holder is this worker's own dead session; it clears within about a minute.""" - sentinel = object() - calls = _acquire_results(monkeypatch, [None, None, sentinel]) - clock = _Clock() - with caplog.at_level("INFO", logger="mergit_worker.startup"): - got = startup.acquire_with_grace( - "postgresql://x", "w", 90.0, sleep=clock.sleep, clock=lambda: clock.now - ) - assert got is sentinel - assert len(calls) == 3 - assert clock.sleeps == [startup.LOCK_RETRY_INTERVAL_S] * 2 - assert "held by another database session" in caplog.text # says what it is waiting for, once - assert caplog.text.count("held by another database session") == 1 - assert "was released" in caplog.text - - -def test_a_lock_that_stays_held_gives_up_after_the_grace_period(monkeypatch: pytest.MonkeyPatch) -> None: - calls = _acquire_results(monkeypatch, []) # always held - clock = _Clock() - assert ( - startup.acquire_with_grace("postgresql://x", "w", 12.0, sleep=clock.sleep, clock=lambda: clock.now) - is None - ) - assert clock.now == 12.0 # never waits past the grace period - assert sum(clock.sleeps) == 12.0 - assert len(calls) == 4 # 0, 5, 10, 12 - - -def test_zero_grace_is_a_single_attempt_with_no_sleep(monkeypatch: pytest.MonkeyPatch) -> None: - calls = _acquire_results(monkeypatch, []) - clock = _Clock() - assert ( - startup.acquire_with_grace("postgresql://x", "w", 0.0, sleep=clock.sleep, clock=lambda: clock.now) - is None - ) - assert len(calls) == 1 and clock.sleeps == [] - - -def test_a_free_lock_is_taken_at_once_without_waiting(monkeypatch: pytest.MonkeyPatch) -> None: - sentinel = object() - calls = _acquire_results(monkeypatch, [sentinel]) - clock = _Clock() - got = startup.acquire_with_grace("postgresql://x", "w", 90.0, sleep=clock.sleep, clock=lambda: clock.now) - assert got is sentinel and len(calls) == 1 and clock.sleeps == [] - - -def test_an_unreachable_database_is_not_waited_out(monkeypatch: pytest.MonkeyPatch) -> None: - def acquire(_cls: object, _url: str, _id: str) -> object: - raise psycopg.OperationalError("connection refused") - - monkeypatch.setattr(startup.WorkerIdLock, "acquire", classmethod(acquire)) # pyright: ignore[reportArgumentType] - clock = _Clock() - with pytest.raises(psycopg.OperationalError): - startup.acquire_with_grace("postgresql://x", "w", 90.0, sleep=clock.sleep, clock=lambda: clock.now) - assert clock.sleeps == [] - - -def test_the_grace_period_defaults_to_ninety_seconds_and_is_configurable( - monkeypatch: pytest.MonkeyPatch, -) -> None: - from mergit_worker.config import WorkerConfig - - assert WorkerConfig.model_fields["worker_id_lock_grace_s"].default == 90.0 - - -def test_a_closed_lock_never_reconnects(monkeypatch: pytest.MonkeyPatch) -> None: - takes: list[str] = [] - monkeypatch.setattr(startup, "_take", lambda _url, key: takes.append(key)) # pyright: ignore[reportUnknownLambdaType] - conn = FakeConn() - conn.fail = True - lock = WorkerIdLock(conn, "postgresql://unused", "mergit_worker:closed") # type: ignore[arg-type] - lock.close() - assert lock.still_held() is None - assert takes == [] diff --git a/control-plane/tests/test_worker_role.py b/control-plane/tests/test_worker_role.py index b738dd76..7140d3fc 100644 --- a/control-plane/tests/test_worker_role.py +++ b/control-plane/tests/test_worker_role.py @@ -7,34 +7,26 @@ from __future__ import annotations import asyncio -import os -import shutil -import subprocess from collections.abc import Iterator from pathlib import Path -from uuid import uuid4 import psycopg import pytest from psycopg import errors -from psycopg.conninfo import conninfo_to_dict, make_conninfo +from psycopg.conninfo import make_conninfo from mergit_cp.db import migrate from mergit_cp.pr_store import _CHANGES, PgPrStore from mergit_worker import jobs as worker_jobs from mergit_worker.jobs import PgJobStore -from mergit_worker.startup import WorkerIdLock, watch_lock +from mergit_worker.startup import WorkerIdLock pytestmark = pytest.mark.postgres GRANTS = Path(__file__).resolve().parents[2] / "deploy" / "worker-vm" / "db-role.sql" -CREATE_ROLE = GRANTS.with_name("create-db-role.sh") -# Roles belong to the whole cluster, not to one database. A name of this run's own, so that dropping it can -# never drop the real mergit_worker (or another run's role) on a cluster the tests were pointed at by mistake. -RUN = uuid4().hex[:8] -ROLE = f"mergit_worker_{RUN}" +ROLE = "mergit_worker" PASSWORD = "role-test-password" -DB = f"worker_role_test_{RUN}" +DB = "worker_role_test" TASK = "0x" + "0" * 64 PLAN = "00000000-0000-0000-0000-000000000000" WORKER_TABLES = {"jobs", "pr_requests", "plans", "tasks"} @@ -46,28 +38,6 @@ async def _migrate(url: str) -> None: await migrate(conn) -def _run_grants(url: str) -> None: - """Run db-role.sql for this run's role, as create-db-role.sh does with WORKER_DB_ROLE.""" - with psycopg.connect(url, autocommit=True) as conn: - conn.execute(f"SET mergit.worker_role = '{ROLE}'") # pyright: ignore[reportArgumentType] - conn.execute(GRANTS.read_text()) # pyright: ignore[reportArgumentType] - - -@pytest.fixture(scope="module", autouse=True) -def _only_a_throwaway_cluster(database_url: str) -> None: - """These tests create and drop roles and databases, which belong to the whole cluster. The - ``database_url`` fixture is a testcontainer on this machine (database ``test``); refuse anything else - unless the operator says so, so a changed fixture or a stray URL can never point this at a real server.""" - if os.environ.get("MERGIT_TEST_ALLOW_ANY_DB") == "1": - return - info = conninfo_to_dict(database_url) - if info.get("host") not in ("localhost", "127.0.0.1", "::1") or info.get("dbname") != "test": - pytest.skip( - "database_url is not a local throwaway test database (host localhost, database 'test'); " - "set MERGIT_TEST_ALLOW_ANY_DB=1 to run against it anyway" - ) - - @pytest.fixture(scope="module") def admin_url(database_url: str) -> Iterator[str]: """A throwaway database with every migration applied and the grants file run, as the real script does.""" @@ -77,8 +47,10 @@ def admin_url(database_url: str) -> Iterator[str]: url = make_conninfo(database_url, dbname=DB) asyncio.run(_migrate(url)) with psycopg.connect(database_url, autocommit=True) as root: + root.execute(f"DROP ROLE IF EXISTS {ROLE}") # a previous run's role, owned by nothing now root.execute(f"CREATE ROLE {ROLE} LOGIN PASSWORD '{PASSWORD}'") - _run_grants(url) + with psycopg.connect(url, autocommit=True) as conn: + conn.execute(GRANTS.read_text()) # pyright: ignore[reportArgumentType] yield url with psycopg.connect(database_url, autocommit=True) as root: root.execute(f"DROP DATABASE {DB} WITH (FORCE)") @@ -181,115 +153,12 @@ def test_running_the_grants_again_takes_extra_rights_away(admin_url: str, role_u conn.execute(f"GRANT SELECT ON sessions TO {ROLE}") with psycopg.connect(role_url, autocommit=True) as conn: conn.execute("SELECT 1 FROM sessions LIMIT 1") # the leak the next run must close - _run_grants(admin_url) + with psycopg.connect(admin_url, autocommit=True) as conn: + conn.execute(GRANTS.read_text()) # pyright: ignore[reportArgumentType] with psycopg.connect(role_url, autocommit=True) as conn, pytest.raises(errors.InsufficientPrivilege): conn.execute("SELECT 1 FROM sessions LIMIT 1") -def test_the_worker_role_has_connect_revoked_from_public( - database_url: str, admin_url: str, role_url: str -) -> None: - """Every role may connect to a new database (PUBLIC has CONNECT); the grants take that away, so a role - made later for something else cannot open the control-plane database unless it is given CONNECT itself.""" - other = f"mergit_other_{RUN}" - with psycopg.connect(database_url, autocommit=True) as root: - root.execute(f"CREATE ROLE {other} LOGIN PASSWORD '{PASSWORD}'") # pyright: ignore[reportArgumentType] - try: - with pytest.raises(psycopg.OperationalError, match="permission denied for database"): - psycopg.connect(make_conninfo(admin_url, user=other, password=PASSWORD)).close() - with psycopg.connect(role_url) as conn: - assert conn.execute("SELECT 1").fetchone() == (1,) - finally: - with psycopg.connect(database_url, autocommit=True) as root: - root.execute(f"DROP ROLE {other}") # pyright: ignore[reportArgumentType] - - -def test_running_the_grants_again_takes_away_default_privileges_on_later_tables( - admin_url: str, role_url: str -) -> None: - """REVOKE ALL ON ALL TABLES covers the tables that exist now. A default privilege (ALTER DEFAULT - PRIVILEGES) would hand every table a later migration creates to the role, or to everyone; the grants must - undo that.""" - with psycopg.connect(admin_url, autocommit=True) as conn: - conn.execute(f"ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT ON TABLES TO {ROLE}") # pyright: ignore[reportArgumentType] - conn.execute("ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES TO PUBLIC") - conn.execute("CREATE TABLE later_leak (secret text)") - try: - with psycopg.connect(role_url, autocommit=True) as conn: - conn.execute("SELECT * FROM later_leak") # the default privileges really did leak it - _run_grants(admin_url) - with psycopg.connect(admin_url, autocommit=True) as conn: - conn.execute("CREATE TABLE later_table (secret text)") - with psycopg.connect(role_url, autocommit=True) as conn: - for table in ("later_leak", "later_table"): - with pytest.raises(errors.InsufficientPrivilege): - conn.execute(f"SELECT * FROM {table}") # pyright: ignore[reportArgumentType] - finally: - with psycopg.connect(admin_url, autocommit=True) as conn: - conn.execute("DROP TABLE IF EXISTS later_leak, later_table") - conn.execute(f"ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE ALL ON TABLES FROM {ROLE}") # pyright: ignore[reportArgumentType] - conn.execute("ALTER DEFAULT PRIVILEGES REVOKE ALL ON TABLES FROM PUBLIC") - - -@pytest.mark.skipif(shutil.which("psql") is None, reason="needs psql on PATH") -def test_the_create_script_makes_the_role_it_is_told_to_with_only_the_grants( - database_url: str, admin_url: str -) -> None: - """create-db-role.sh as shipped, through psql, with WORKER_DB_ROLE naming a role of this run's own.""" - name = f"mergit_script_{RUN}" - password = "script-test-password-0123456789" - target = database_url.rsplit("/", 1)[0] + f"/{DB}" # a URI: PSQL is split on spaces - env = { - "PATH": os.environ["PATH"], - "PSQL": f"psql -X {target} -v ON_ERROR_STOP=1 -q", - "WORKER_DB_ROLE": name, - "WORKER_DB_PASSWORD": password, - } - try: - run = subprocess.run(["bash", str(CREATE_ROLE)], env=env, capture_output=True, text=True, check=False) - assert run.returncode == 0, run.stderr - assert f"postgres://{name}:" in run.stdout - url = make_conninfo(admin_url, user=name, password=password) - with psycopg.connect(url, autocommit=True) as conn: - assert conn.execute("SELECT count(*) FROM jobs").fetchone() is not None - with pytest.raises(errors.InsufficientPrivilege): - conn.execute("SELECT 1 FROM sessions LIMIT 1") - finally: - with psycopg.connect(admin_url, autocommit=True) as conn: - if conn.execute("SELECT 1 FROM pg_roles WHERE rolname = %s", (name,)).fetchone(): - conn.execute(f"DROP OWNED BY {name}") # its grants, here and on the database # pyright: ignore[reportArgumentType] - conn.execute(f"DROP ROLE {name}") # pyright: ignore[reportArgumentType] - - -@pytest.mark.skipif(shutil.which("psql") is None, reason="needs psql on PATH") -def test_create_db_role_refuses_a_superuser_name(database_url: str, admin_url: str) -> None: - """WORKER_DB_ROLE naming an existing superuser must not reset its password or print a URL for it. (The - names `postgres` and `pg_*` are refused before psql runs: tests/test_deploy_scripts.py.)""" - name = f"mergit_super_{RUN}" - target = database_url.rsplit("/", 1)[0] + f"/{DB}" - with psycopg.connect(admin_url, autocommit=True) as conn: - conn.execute(f"CREATE ROLE {name} SUPERUSER LOGIN PASSWORD 'old-password-0123456789'") # pyright: ignore[reportArgumentType] - before = conn.execute("SELECT rolpassword FROM pg_authid WHERE rolname = %s", (name,)).fetchone() - env = { - "PATH": os.environ["PATH"], - "PSQL": f"psql -X {target} -v ON_ERROR_STOP=1 -q", - "WORKER_DB_ROLE": name, - "WORKER_DB_PASSWORD": "new-password-0123456789abcdef", - } - try: - run = subprocess.run(["bash", str(CREATE_ROLE)], env=env, capture_output=True, text=True, check=False) - assert run.returncode != 0 - assert "superuser" in run.stderr - assert "postgres://" not in run.stdout and "new-password" not in run.stdout + run.stderr - with psycopg.connect(admin_url) as conn: # the password was not reset - after = conn.execute("SELECT rolpassword FROM pg_authid WHERE rolname = %s", (name,)).fetchone() - assert after == before - finally: - with psycopg.connect(admin_url, autocommit=True) as conn: - conn.execute(f"DROP OWNED BY {name}") # pyright: ignore[reportArgumentType] - conn.execute(f"DROP ROLE {name}") # pyright: ignore[reportArgumentType] - - # -- one process per worker id -------------------------------------------------------------------------- @@ -323,7 +192,11 @@ def test_a_killed_worker_does_not_block_its_own_restart(database_url: str) -> No """A SIGKILLed container never calls close(); Postgres drops the session and with it the lock.""" dead = WorkerIdLock.acquire(database_url, "killed-id") assert dead is not None - _kill_lock_session(database_url, dead) + with psycopg.connect(database_url, autocommit=True) as admin: + admin.execute( + "SELECT pg_terminate_backend(pid) FROM pg_stat_activity" + " WHERE pid <> pg_backend_pid() AND query LIKE '%pg_try_advisory_lock%'" + ) restarted = None for _ in range(50): # the server notices the dropped connection almost at once restarted = WorkerIdLock.acquire(database_url, "killed-id") @@ -338,149 +211,3 @@ def test_the_role_can_take_the_lock_too(role_url: str) -> None: lock = WorkerIdLock.acquire(role_url, "role-lock-id") assert lock is not None lock.close() - - -def test_the_lock_connection_has_tcp_keepalives_on_both_ends(role_url: str) -> None: - """After a worker VM loses power the server keeps its backend, and the lock, until the server's own - keepalive gives up: by default about two hours, during which the restarted worker cannot start. The - lock session lowers that to about a minute; and the role may set it, which is the part that can fail.""" - lock = WorkerIdLock.acquire(role_url, "keepalive-id") - assert lock is not None - try: - conn = lock._conn # pyright: ignore[reportPrivateUsage] - assert conn is not None - options = conn.pgconn.info - client = {o.keyword.decode(): (o.val or b"").decode() for o in options if o.keyword != b"password"} - assert { - k: client[k] for k in ("keepalives", "keepalives_idle", "keepalives_interval", "keepalives_count") - } == { - "keepalives": "1", - "keepalives_idle": "30", - "keepalives_interval": "10", - "keepalives_count": "3", - } - assert client["tcp_user_timeout"] == "60000" - assert client["connect_timeout"] == "10" - # pg_settings.setting is the raw value in the setting's base unit (s, s, none, ms). current_setting() - # and SHOW may render it with a unit ("30s", "1min"), which would not compare to the numbers. - rows = conn.execute( - "SELECT name, setting FROM pg_settings WHERE name = ANY(%s)", - (["tcp_keepalives_idle", "tcp_keepalives_interval", "tcp_keepalives_count", "tcp_user_timeout"],), - ).fetchall() - server = {str(name): str(setting) for name, setting in rows} - assert server == { # the test container is reached over TCP; on a Unix socket these read 0 - "tcp_keepalives_idle": "30", - "tcp_keepalives_interval": "10", - "tcp_keepalives_count": "3", - "tcp_user_timeout": "60000", - } - finally: - lock.close() - - -# -- the lock survives a database restart, and a dead database is a clean exit --------------------------- - - -def _terminate(database_url: str, pid: int) -> None: - """End one backend and wait (up to 5 s) until it has exited, so its advisory lock is released.""" - with psycopg.connect(database_url, autocommit=True) as admin: - row = admin.execute("SELECT pg_terminate_backend(%s, 5000)", (pid,)).fetchone() - assert row == (True,) - - -def _kill_lock_session(database_url: str, lock: WorkerIdLock) -> None: - """End the session holding this lock, found by its pid: matching on the query text would also end any - other backend whose last query mentions advisory locks, and misses this one once it has run anything - else.""" - conn = lock._conn # pyright: ignore[reportPrivateUsage] - assert conn is not None - _terminate(database_url, conn.info.backend_pid) - - -def test_the_lock_is_taken_again_after_the_database_drops_the_session(database_url: str) -> None: - """What a cp_postgres restart does to a live worker: the session and its lock vanish. Without this the - process runs on unguarded and a second one with the same id would fail its jobs. - - The session is ended by its pid: after still_held() its last query is ``SELECT 1``, so matching on - ``pg_try_advisory_lock`` would end nothing and the reconnect below would never be exercised.""" - lock = WorkerIdLock.acquire(database_url, "restart-held-id") - assert lock is not None - try: - assert lock.still_held() is True - old = lock._conn # pyright: ignore[reportPrivateUsage] - assert old is not None - old_pid = old.info.backend_pid - _terminate(database_url, old_pid) - assert lock.still_held() is True # reconnected and locked again - new = lock._conn # pyright: ignore[reportPrivateUsage] - assert new is not None and new is not old and old.closed - assert new.info.backend_pid != old_pid # a new session, not the one that was ended - assert WorkerIdLock.acquire(database_url, "restart-held-id") is None # and it really is held - finally: - lock.close() - - -def test_a_lock_another_process_took_while_the_database_was_down_is_reported_lost(database_url: str) -> None: - lock = WorkerIdLock.acquire(database_url, "stolen-id") - assert lock is not None - thief = None - try: - _kill_lock_session(database_url, lock) - for _ in range(50): # the drop is noticed almost at once; the thief takes the id - thief = WorkerIdLock.acquire(database_url, "stolen-id") - if thief is not None: - break - asyncio.run(asyncio.sleep(0.1)) - assert thief is not None - assert lock.still_held() is False - finally: - lock.close() - if thief is not None: - thief.close() - - -def test_an_unreachable_database_is_unknown_not_lost(database_url: str) -> None: - lock = WorkerIdLock.acquire(database_url, "down-id") - assert lock is not None - try: - lock._url = make_conninfo(database_url, port=1, connect_timeout=1) # pyright: ignore[reportPrivateUsage] - _kill_lock_session(database_url, lock) - assert lock.still_held() is None # nothing can have taken the id from a database that is down - finally: - lock.close() - - -def test_the_watchdog_calls_on_lost_once_and_stops(database_url: str) -> None: - import threading - - class Lost: - def still_held(self) -> bool: - return False - - calls: list[int] = [] - watch_lock(Lost(), threading.Event(), 0.01, lambda: calls.append(1)) # type: ignore[arg-type] - assert calls == [1] - - -def test_a_database_that_is_down_at_start_exits_cleanly_without_a_traceback( - monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture -) -> None: - from types import SimpleNamespace - - from pydantic import SecretStr - - import mergit_worker.__main__ as entry - - cfg = SimpleNamespace( - worker_id="boot-id", - agents_file=Path("unused"), - require_all_agent_keys=False, - worker_id_lock_grace_s=0.0, - database_url=SecretStr("postgresql://nobody@127.0.0.1:1/none?connect_timeout=1"), - ) - monkeypatch.setattr(entry, "load_config", lambda: cfg) - monkeypatch.setattr(entry, "agents_without_key", lambda _path: []) - with caplog.at_level("ERROR"): - assert entry.main() == 1 - assert "cannot reach the database at start-up" in caplog.text - assert not any(r.exc_info for r in caplog.records) diff --git a/deploy/compose.controlplane.yaml b/deploy/compose.controlplane.yaml index 1c12347d..9c046231 100644 --- a/deploy/compose.controlplane.yaml +++ b/deploy/compose.controlplane.yaml @@ -1,26 +1,28 @@ # The orchestration API and the agent worker (plan task O10) on top of the testnet stack. -# deploy/make-controlplane-env.sh [app-domain] # once: writes deploy/secrets/{api,worker,cp_postgres}.env +# deploy/make-controlplane-env.sh [app-domain] # once: writes deploy/secrets/api.env and worker.env # docker build -t mergit-sandbox:dev control-plane/sandbox # sudo install -d /var/lib/mergit/workspaces # docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml up -d --build # On the server add `-f deploy/compose.server.yaml` last (Caddy then proxies api.mergit.site to `api`). # # The API and the worker get separate secret files so each holds only what it needs: neither has the gateway or -# treasury key, and only the worker has an agent runtime key and the Docker socket. The superuser password is -# generated by that script too: cp_postgres reads it from secrets/cp_postgres.env, and the API and the worker from -# the DATABASE_URL in their own files. Compose stops with "env file ... not found" until the files exist; a server -# set up before the password moved there needs the one-time step in deploy/server/README.md. +# treasury key, and only the worker has an agent runtime key and the Docker socket. + +# Pinned so deploy/postgres/pg_hba.conf can match it by an exact CIDR instead of `samenet`, which can be widened by +# how a given host's Docker handles published ports (see the comment there). Change both together. +networks: + default: + ipam: + config: [{ subnet: 172.30.9.0/24 }] services: cp_postgres: image: postgres:16 restart: unless-stopped # The control plane does not share the gateway's database: both define tables named plans, tasks and outbox. - environment: { POSTGRES_DB: mergit_cp } - env_file: secrets/cp_postgres.env - # deploy/postgres/pg_hba.conf: from outside the containers' address range (`networks:` below), only the - # mergit_worker role may log in. The folder is mounted, not the file, so a `git pull` that replaces the file is - # seen (apply it with a restart). + environment: { POSTGRES_PASSWORD: mergit, POSTGRES_DB: mergit_cp } + # deploy/postgres/pg_hba.conf: from outside this compose network, only the mergit_worker role may log in. The + # folder is mounted, not the file, so a `git pull` that replaces the file is seen (apply it with a restart). command: ["postgres", "-c", "hba_file=/etc/postgresql/mergit/pg_hba.conf"] volumes: ["cp_pgdata:/var/lib/postgresql/data", "./postgres:/etc/postgresql/mergit:ro"] healthcheck: { test: ["CMD", "pg_isready", "-U", "postgres"], interval: 2s, retries: 30 } @@ -54,8 +56,9 @@ services: image: mergit-api:local restart: unless-stopped env_file: secrets/api.env - # Loopback only: a port published on 0.0.0.0 skips the host firewall and would bypass the Caddy lockdown - # (compose.server.yaml drops it entirely; Caddy reaches the API by name). The healthcheck runs inside the container. + # Loopback only: on the server, compose.server.yaml resets this to nothing and Caddy reaches `api` by name + # instead (deploy/Caddyfile). Published to all interfaces this would be the unproxied API, open to the + # internet and skipping the Caddy lockdown, for a run without compose.server.yaml. ports: ["127.0.0.1:8000:8000"] depends_on: cp_postgres: { condition: service_healthy } @@ -69,7 +72,8 @@ services: timeout: 5s retries: 3 start_period: 40s - environment: # DATABASE_URL is in secrets/api.env + environment: + DATABASE_URL: postgres://postgres:mergit@cp_postgres:5432/mergit_cp GATEWAY_URL: http://gateway:8082 STORE_URL: http://store:8081 AGENTS_FILE: /agents.json @@ -90,7 +94,8 @@ services: - /var/lib/mergit/workspaces:/var/lib/mergit/workspaces - ../../mergit-contracts/deployments/base-sepolia.json:/deployments/base-sepolia.json:ro - ../../mergit-contracts/deployments/agents-base-sepolia.json:/agents.json:ro - environment: # DATABASE_URL is in secrets/worker.env + environment: + DATABASE_URL: postgres://postgres:mergit@cp_postgres:5432/mergit_cp GATEWAY_URL: http://gateway:8082 STORE_URL: http://store:8081 DEPLOYMENT_FILE: /deployments/base-sepolia.json @@ -100,21 +105,3 @@ services: SANDBOX_WORKSPACE_ROOT: /var/lib/mergit/workspaces volumes: { cp_pgdata: {} } - -# The compose network's addresses, fixed so deploy/postgres/pg_hba.conf can name them: the superuser may log in from -# ip_range and loopback only (tests/test_cp_postgres_hba.py checks the two files agree). The gateway is the host's -# end of the bridge, and it is deliberately OUTSIDE ip_range. Docker usually keeps the source address of a connection -# to a published port, but not always: when docker-proxy (the userland proxy, on by default) carries it, with rootless -# Docker, or with iptables off, the worker VM's connection reaches cp_postgres from the gateway's address, so a rule -# covering the whole subnet would let the worker VM in as postgres. -# Containers get .128-.254, so 126 of them fit. 172.30.53.0/24 was picked to stay clear of Docker's automatic pools -# in normal use (172.17-172.29 are handed out first) and of the 10.x ranges cloud private networks use. If it clashes -# with a network on the server (`docker network create` then fails with "Pool overlaps"), change all three values -# here AND the matching line in pg_hba.conf. Changing them needs `docker compose down` (volumes are kept), then `up`. -networks: - default: - ipam: - config: - - subnet: 172.30.53.0/24 - gateway: 172.30.53.1 - ip_range: 172.30.53.128/25 diff --git a/deploy/make-controlplane-env.sh b/deploy/make-controlplane-env.sh index 2d54afa3..933dc830 100755 --- a/deploy/make-controlplane-env.sh +++ b/deploy/make-controlplane-env.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Writes deploy/secrets/api.env, worker.env and cp_postgres.env (mode 600, git-ignored) for compose.controlplane.yaml. +# Writes deploy/secrets/api.env and deploy/secrets/worker.env (mode 600, git-ignored) for compose.controlplane.yaml. # deploy/make-controlplane-env.sh [app-domain] default mergit.site. On a laptop pass the page's host WITH its port, # e.g. localhost:5173: sign-in checks it against window.location.host. # The service tokens are copied from deploy/secrets/testnet.env (run make-testnet-env.sh first) so the gateway and the @@ -7,24 +7,13 @@ # $ANTHROPIC_API_KEY / $GROQ_API_KEY or asked for without echo; give either or both (Enter skips one). Anthropic is used # when its key is present, Groq when only Groq's is (control-plane/mergit_worker/providers.py). Optional: GROQ_MODEL # (default openai/gpt-oss-120b) and LLM_PROVIDER=anthropic|groq to force one. Prints names only, never values. -# The cp_postgres superuser password is generated here: cp_postgres.env sets it when the database is first created, -# and the API and the worker get it in DATABASE_URL. Postgres reads POSTGRES_PASSWORD only when its volume is empty, -# so for a database that already exists see deploy/server/README.md ("The database superuser password"). set -euo pipefail cd "$(dirname "$0")" domain=${1:-mergit.site} svc=../../mergit-contracts/.env.services [ -e secrets/testnet.env ] || { echo "secrets/testnet.env missing: run make-testnet-env.sh first" >&2; exit 1; } [ -e "$svc" ] || { echo "$svc missing" >&2; exit 1; } -for f in api.env worker.env cp_postgres.env; do - [ ! -e "secrets/$f" ] || { - echo "secrets/$f exists; delete api.env, worker.env and cp_postgres.env to regenerate." >&2 - echo "WARNING: if this server's cp_pgdata volume already exists, regenerating writes a database password the" >&2 - echo "database never receives, and the API and the worker are locked out. Use the existing-database procedure in" >&2 - echo "deploy/server/README.md (\"The database superuser password\") instead." >&2 - exit 1 - } -done +{ [ ! -e secrets/api.env ] && [ ! -e secrets/worker.env ]; } || { echo "secrets/api.env or worker.env exists; delete them to regenerate" >&2; exit 1; } token() { grep -E "^$1=.+" secrets/testnet.env | head -1 | cut -d= -f2-; } for t in EVENTS_TOKEN CHAIN_EVENTS_TOKEN WORKER_TOKEN STORE_WRITE_TOKEN; do @@ -47,16 +36,10 @@ model_keys() { # only the keys that were given, so the provider is chosen from w } session=$(head -c 32 /dev/urandom | od -An -tx1 | tr -d ' \n') [ ${#session} -eq 64 ] || { echo "could not generate SESSION_SECRET" >&2; exit 1; } -# Hex, so it needs no escaping in a URL. -pg_password=$(head -c 24 /dev/urandom | od -An -tx1 | tr -d ' \n') -[ ${#pg_password} -eq 48 ] || { echo "could not generate the database password" >&2; exit 1; } -database_url="postgres://postgres:$pg_password@cp_postgres:5432/mergit_cp" scheme=https; case "$domain" in localhost*|127.0.0.1*) scheme=http ;; esac umask 077 -echo "POSTGRES_PASSWORD=$pg_password" > secrets/cp_postgres.env { - echo "DATABASE_URL=$database_url" echo "APP_DOMAIN=$domain" echo "CORS_ORIGINS=$scheme://$domain" echo "SESSION_SECRET=$session" @@ -65,13 +48,11 @@ echo "POSTGRES_PASSWORD=$pg_password" > secrets/cp_postgres.env for t in EVENTS_TOKEN CHAIN_EVENTS_TOKEN WORKER_TOKEN; do echo "$t=$(token $t)"; done } > secrets/api.env { - echo "DATABASE_URL=$database_url" model_keys echo "MERGIT_AGENT_KEY_1=$agent_key" echo "APP_BASE_URL=$scheme://$domain" for t in EVENTS_TOKEN STORE_WRITE_TOKEN WORKER_TOKEN; do echo "$t=$(token $t)"; done # GITHUB_TOKEN=... (optional: only the pull-request gate needs it; add it here by hand) } > secrets/worker.env -echo "wrote deploy/secrets/cp_postgres.env (POSTGRES_PASSWORD)" -echo "wrote deploy/secrets/api.env (DATABASE_URL APP_DOMAIN CORS_ORIGINS SESSION_SECRET PLANNER_MODEL, the model key(s) you gave, EVENTS_TOKEN CHAIN_EVENTS_TOKEN WORKER_TOKEN)" -echo "wrote deploy/secrets/worker.env (DATABASE_URL, the model key(s) you gave, MERGIT_AGENT_KEY_1 APP_BASE_URL EVENTS_TOKEN STORE_WRITE_TOKEN WORKER_TOKEN)" +echo "wrote deploy/secrets/api.env (APP_DOMAIN CORS_ORIGINS SESSION_SECRET PLANNER_MODEL, the model key(s) you gave, EVENTS_TOKEN CHAIN_EVENTS_TOKEN WORKER_TOKEN)" +echo "wrote deploy/secrets/worker.env (the model key(s) you gave, MERGIT_AGENT_KEY_1 APP_BASE_URL EVENTS_TOKEN STORE_WRITE_TOKEN WORKER_TOKEN)" diff --git a/deploy/postgres/pg_hba.conf b/deploy/postgres/pg_hba.conf index 088309eb..32d4628d 100644 --- a/deploy/postgres/pg_hba.conf +++ b/deploy/postgres/pg_hba.conf @@ -1,18 +1,16 @@ # Who may log in to cp_postgres (deploy/compose.controlplane.yaml). It replaces the image's default, whose last line, -# `host all all all scram-sha-256`, lets the superuser in from any address that reaches the port. Once CP_DB_BIND -# publishes the port on the private network (deploy/worker-vm), that address would include the worker VM, the machine -# most likely to be compromised, and the restricted role would be pointless. So: the containers on this compose -# network may use any role, and anything else only mergit_worker. +# `host all all all scram-sha-256`, lets the superuser in from any address that reaches the port, with the password +# in compose.controlplane.yaml. Once CP_DB_BIND publishes the port on the private network (deploy/worker-vm), that +# address would include the worker VM, the machine most likely to be compromised, and the restricted role would be +# pointless. So: the containers on this compose network may use any role, and anything else only mergit_worker. # -# "The containers" is 172.30.53.128/25, the ip_range of the compose network (`networks:` at the end of -# compose.controlplane.yaml; tests/test_cp_postgres_hba.py checks the two agree). It is NOT `samenet`, and not the -# whole 172.30.53.0/24: both include the bridge gateway 172.30.53.1, the host's own address on the network. Docker -# usually keeps the source address of a connection to a published port, but not always: when docker-proxy (the -# userland proxy, on by default) carries it, with rootless Docker (its port driver), or with `"iptables": false` in -# daemon.json, the worker VM's connection reaches cp_postgres from the gateway, so a rule covering the gateway would -# let it log in as postgres. Nothing in this file depends on which of those the server uses. -local all all trust -host all all 127.0.0.1/32 trust -host all all ::1/128 trust -host all all 172.30.53.128/25 scram-sha-256 -host mergit_cp mergit_worker all scram-sha-256 +# The compose network is pinned to the subnet below (deploy/compose.controlplane.yaml, `networks.default.ipam`) +# and matched here by that exact CIDR, not `samenet`: `samenet` matches any address sharing a subnet with any of +# the server's interfaces, which under some Docker network settings (docker-proxy's own bridge gateway, +# `iptables: false`) is the address a published port sees a connection arrive from, superuser password and all. +# A fixed, narrow CIDR cannot widen that way. Keep the two in step if the subnet is ever changed. +local all all trust +host all all 127.0.0.1/32 trust +host all all ::1/128 trust +host all all 172.30.9.0/24 scram-sha-256 +host mergit_cp mergit_worker all scram-sha-256 diff --git a/deploy/server/README.md b/deploy/server/README.md index c7184079..bede8a46 100644 --- a/deploy/server/README.md +++ b/deploy/server/README.md @@ -10,7 +10,7 @@ One server (Hetzner CPX32, Nuremberg) runs everything that used to run on a lapt | `store.mergit.site` | evidence store (receipt bundles) | Caddy, then `store:8081` | | _(not public)_ | validator, Postgres | compose network only | | `app.mergit.site/v0/{auth,account,plans,agents}` | orchestration API (control plane), same origin as the app | Caddy, then `api:8000`; needs `deploy/compose.controlplane.yaml` | -| `api.mergit.site` | the same API on its own name: the non-account browser routes, `/healthz`, and `/v0/internal/*` from the worker VM only (`WORKER_VM_IP`); everything else 404 | Caddy, then `api:8000`; needs an A record | +| `api.mergit.site` | the same API on its own name: browser routes, `/healthz`, and `/v0/internal/*` from the worker VM only (`WORKER_VM_IP`); everything else 404 | Caddy, then `api:8000`; needs an A record | | _(its own VM)_ | the agent worker | `deploy/worker-vm/README.md` | Everything is testnet money. The Hetzner firewall (`mergit-fw`) allows only 22, 80 and 443. @@ -40,12 +40,6 @@ ssh viscous@SERVER 'chmod 600 ~/mergit/mergit/deploy/secrets/testnet.env' **Stop the old stack first** (`docker compose -f deploy/compose.testnet.yaml down` on the laptop). Two gateways sending transactions with the same key collide on nonces. -Then, on the server, `deploy/make-controlplane-env.sh` writes the control plane's secrets: `api.env`, `worker.env` and -`cp_postgres.env`. The last holds the `cp_postgres` superuser password it generates (the API and the worker get the same -password in their `DATABASE_URL`). Compose refuses to start until these files exist. A server set up before the -password moved out of `compose.controlplane.yaml` needs the one-time step in -[The database superuser password](#the-database-superuser-password) instead. - ## 3. DNS (Spaceship, Advanced DNS) Add A records to the server's IPv4 address for `gateway` and `store` now; they are new names, so nothing breaks. @@ -61,8 +55,6 @@ docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yam docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml -f deploy/compose.server.yaml exec caddy caddy validate --config /etc/caddy/Caddyfile ``` -If a worker VM will connect, run `deploy/worker-vm/create-db-role.sh` only now that the stack is up (it needs `cp_postgres` running and the tables the API creates); see `deploy/worker-vm/README.md`. - Check the services once `gateway.` and `store.` resolve: ```bash @@ -113,79 +105,6 @@ notice and deploys nothing. Two consequences: a push that touches neither workfl does not deploy by itself, and a manual run from the Actions tab skips the checks. A build that fails on the server leaves the running containers as they were, but a change that builds and misbehaves still goes live. -## The database superuser password - -`cp_postgres` once ran with the superuser password `mergit`, written in `compose.controlplane.yaml`. It now comes from -`deploy/secrets/cp_postgres.env`, which `make-controlplane-env.sh` generates. Postgres reads `POSTGRES_PASSWORD` only when -it creates a new database, so on a server whose `cp_pgdata` volume already exists the password must also be changed -inside the database. Once per server, after pulling this change (until then `update.sh` stops at "env file ... not -found" and the running containers stay as they are). Each step below is safe to stop at; nothing changes in the running -database or stack until step 3. - -**Step 1: check that the new network range is free.** The compose network now has a fixed address range (`networks:` in -`compose.controlplane.yaml`, which `deploy/postgres/pg_hba.conf` names). This prints a line only when another Docker -network on the server uses `172.30.53.0/24`; if it does, stop here and change the range (see the end of this section): - -```bash -docker network inspect $(docker network ls -q) -f '{{range .IPAM.Config}}{{.Subnet}} {{end}}' | tr ' ' '\n' | grep -x 172.30.53.0/24 -``` - -**Step 2: make the password file and build the new images.** Compose will not load without `cp_postgres.env`, so it is -written first. Building now means the restart in step 3 takes seconds, not the minutes a Rust build takes. The running -containers and the database are untouched: - -```bash -cd ~/mergit/mergit -bash -euo pipefail <<'EOF' -cd deploy -[ -e secrets/api.env ] && [ -e secrets/worker.env ] || { echo "no api.env/worker.env: run make-controlplane-env.sh instead" >&2; exit 1; } -[ ! -e secrets/cp_postgres.env ] || { echo "secrets/cp_postgres.env exists already" >&2; exit 1; } -! grep -q '^DATABASE_URL=' secrets/api.env secrets/worker.env || { echo "api.env or worker.env has a DATABASE_URL already" >&2; exit 1; } -umask 077 -echo "POSTGRES_PASSWORD=$(head -c 24 /dev/urandom | od -An -tx1 | tr -d ' \n')" > secrets/cp_postgres.env -echo "wrote secrets/cp_postgres.env" -EOF -docker compose -f deploy/compose.testnet.yaml -f deploy/compose.controlplane.yaml -f deploy/compose.server.yaml build # minutes -``` - -If the build fails, fix the cause and run only the `build` line again; the script above refuses to run twice. - -**Step 3: change the password and restart at once.** This is the only step that changes anything live. Between the -`ALTER ROLE` and the restart, the running API's new database connections fail to authenticate (it still holds the old -password in its environment; connections it already has stay open), so run the restart straight after. `down` keeps the -volumes; the sites are down for the seconds between `down` and `up` (the images are already built), and the worker VM -reconnects by itself. Use `up -d --build` (or `deploy/server/update.sh`): a plain `up -d` would start the old images. -If the `ALTER ROLE` fails the script stops before touching `api.env` and `worker.env`; fix the cause and run it again. If it stops after the `DATABASE_URL` lines are written (the script then refuses to -run again), the password is already changed: run just the `down` and `up -d --build` lines. - -```bash -bash -euo pipefail <<'EOF' -cd ~/mergit/mergit/deploy -c() { docker compose -f compose.testnet.yaml -f compose.controlplane.yaml -f compose.server.yaml "$@"; } -! grep -q '^DATABASE_URL=' secrets/api.env secrets/worker.env || { echo "api.env or worker.env has a DATABASE_URL already" >&2; exit 1; } -pw=$(sed -n 's/^POSTGRES_PASSWORD=//p' secrets/cp_postgres.env) -[ ${#pw} -eq 48 ] || { echo "secrets/cp_postgres.env has no generated password: step 2 first" >&2; exit 1; } -# The password goes to psql on stdin, never on a command line, where `ps` would show it. -{ printf "\\\\set pw '%s'\n" "$pw"; echo "ALTER ROLE postgres PASSWORD :'pw';"; } \ - | c exec -T cp_postgres psql -U postgres -v ON_ERROR_STOP=1 -q -umask 077 -for f in api worker; do echo "DATABASE_URL=postgres://postgres:$pw@cp_postgres:5432/mergit_cp" >> "secrets/$f.env"; done -echo "changed the password and added DATABASE_URL to api.env and worker.env; restarting" -c down -c up -d --build -EOF -``` - -If `up` fails with "Pool overlaps with other one on this address space" (step 1 missed it), another Docker network uses -`172.30.53.0/24`: change the three addresses in `compose.controlplane.yaml` and the matching line in `pg_hba.conf` -together (`tests/test_cp_postgres_hba.py` checks they agree), then run `up -d --build` again. - -The `networks:` block pins the compose network's subnet, and Docker cannot change the subnet of a network that already -exists. On a server whose stack was started before this change, the old network keeps its automatic subnet, so -`up` can fail (or leave `cp_postgres` outside the range `pg_hba.conf` allows, so the API cannot log in). Recreate it once: -`docker compose ... down` (keeps the volumes), `docker network prune` if the old network is left, then `up -d --build`. -Step 3 above does the `down` already; a deploy by `update.sh` alone does not. - ## Notes - Data lives in Docker volumes (`pgdata`, `receipts`, `audits`, `caddy_data`). `docker compose down` keeps them; @@ -206,7 +125,6 @@ deploy/server/backup.sh # writes ~/backups/mergit_cp-