From f6e20b5e0c7a95872bdc214951cccf52e911e3ba Mon Sep 17 00:00:00 2001 From: tegwick Date: Tue, 8 Sep 2026 10:11:35 +0200 Subject: [PATCH] Survive credential rotation: re-read the lease for every connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deployment ran for one lease window and then sat unready for eight hours. Platform credentials are 30-minute leases, not passwords: the service read the mounted URL once at start-up, so External Secrets kept the file current while the engine held the URL it booted with, and every reconnection after the first expiry used a credential the database had already revoked. make_engine now takes an optional refresh callable, invoked by a do_connect hook each time the pool opens a connection, and pool_recycle is 900s so a pooled connection is retired well inside the lease. Only username and password are taken from the refreshed URL — host, port and database come from the engine, so a malformed refresh cannot silently redirect the service somewhere else. Two things behaved correctly and are worth keeping. /readyz reported the real cause, "database unreachable: OperationalError", rather than a generic failure. And liveness stayed independent of the database, so the pod was never restart-looped: it was alive, unable to serve, and said so. Pointing liveness at a database-dependent path would have masked this as a crash loop. Service tests 47 -> 49, including one asserting pool_recycle stays inside the shortest lease the platform issues. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bjefh8NUiEiahN4JLwoSKM Assistant: claude-code Assistant-Model: opus Assistant-Process: 388925@bnt-lap001 Assistant-Session: 3507023f-e0fd-4a1e-9d90-a0d4217d1502 --- service/pyproject.toml | 2 +- .../src/canned_prompts_service/__init__.py | 2 +- service/src/canned_prompts_service/api.py | 7 ++- service/src/canned_prompts_service/db.py | 48 ++++++++++++++++--- service/tests/test_health.py | 26 ++++++++++ 5 files changed, 76 insertions(+), 9 deletions(-) diff --git a/service/pyproject.toml b/service/pyproject.toml index 589d3fe..dc08b87 100644 --- a/service/pyproject.toml +++ b/service/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "canned-prompts-service" -version = "0.1.4" +version = "0.1.5" description = "Hosted registry and index service for Canned Prompt Format packages" requires-python = ">=3.12" dependencies = [ diff --git a/service/src/canned_prompts_service/__init__.py b/service/src/canned_prompts_service/__init__.py index 3870b88..b300a9a 100644 --- a/service/src/canned_prompts_service/__init__.py +++ b/service/src/canned_prompts_service/__init__.py @@ -8,4 +8,4 @@ boundary holds, so rendering stays deterministic and a `derive` default remains a declaration the service does not satisfy. """ -__version__ = "0.1.4" +__version__ = "0.1.5" diff --git a/service/src/canned_prompts_service/api.py b/service/src/canned_prompts_service/api.py index eb0dace..ebda8cc 100644 --- a/service/src/canned_prompts_service/api.py +++ b/service/src/canned_prompts_service/api.py @@ -24,7 +24,12 @@ from .settings import Settings, get_settings def create_app(settings: Settings | None = None, engine: Engine | None = None) -> FastAPI: settings = settings or get_settings() if engine is None and settings.configured: - engine = make_engine(settings.resolved_database_url) + # Pass the resolver, not just the value: the credential is a lease and + # the mounted file is refreshed under us. + engine = make_engine( + settings.resolved_database_url, + refresh=lambda: settings.resolved_database_url, + ) app = FastAPI(title="canned-prompts registry", version=__version__) app.state.settings = settings diff --git a/service/src/canned_prompts_service/db.py b/service/src/canned_prompts_service/db.py index 1482320..2f88c7d 100644 --- a/service/src/canned_prompts_service/db.py +++ b/service/src/canned_prompts_service/db.py @@ -11,8 +11,10 @@ from dataclasses import dataclass import os import re -from sqlalchemy import create_engine, text -from sqlalchemy.engine import Engine +from typing import Callable + +from sqlalchemy import create_engine, event, text +from sqlalchemy.engine import Engine, make_url from sqlalchemy.orm import DeclarativeBase, Session, sessionmaker @@ -20,10 +22,44 @@ class Base(DeclarativeBase): pass -def make_engine(database_url: str) -> Engine: - # future=True is the 2.0 default; pool_pre_ping keeps a recycled connection - # from surfacing as a request error after a database restart. - return create_engine(database_url, pool_pre_ping=True, future=True) +# Well under the shortest credential lease the platform issues (30 minutes for +# the runtime role), so a pooled connection is retired before its credential +# expires rather than failing mid-request. +POOL_RECYCLE_SECONDS = 900 + + +def make_engine(database_url: str, refresh: "Callable[[], str] | None" = None) -> Engine: + """Build the engine, re-reading credentials for every new connection. + + `refresh`, when given, is called each time the pool opens a DBAPI + connection and returns the *current* URL. + + This matters because platform credentials are short leases, not passwords. + Reading the mounted file once at start-up worked for exactly one lease + window and then failed permanently: External Secrets kept the file current, + the engine kept the URL it booted with, and every reconnection used a + credential the database had already revoked. + """ + engine = create_engine(database_url, pool_pre_ping=True, future=True, + pool_recycle=POOL_RECYCLE_SECONDS) + if refresh is None: + return engine + + @event.listens_for(engine, "do_connect") + def _use_current_credentials(dialect, conn_rec, cargs, cparams): # noqa: ANN001 + current = refresh() + if not current: + return None + url = make_url(current) + # Only the parts a lease rotates. Host, port and database come from the + # engine so a malformed refresh cannot silently redirect the service. + if url.username: + cparams["user"] = url.username + if url.password: + cparams["password"] = url.password + return None + + return engine def make_session_factory(engine: Engine) -> sessionmaker[Session]: diff --git a/service/tests/test_health.py b/service/tests/test_health.py index 3ef50d5..26bd82d 100644 --- a/service/tests/test_health.py +++ b/service/tests/test_health.py @@ -138,3 +138,29 @@ def test_absent_database_file_still_fails_loudly(tmp_path: Path) -> None: settings = Settings(database_url_file=str(tmp_path / "absent")) with pytest.raises(RuntimeError, match="cannot read secret file"): _ = settings.resolved_database_url + + +def test_engine_rereads_credentials_on_each_connection(tmp_path: Path) -> None: + """Platform credentials are 30-minute leases, not passwords. Reading the + file once at start-up worked for one lease window and then failed + permanently — External Secrets kept the file current while the engine kept + the URL it booted with.""" + secret = tmp_path / "url" + secret.write_text(f"sqlite:///{tmp_path / 'a.db'}", encoding="utf-8") + settings = Settings(database_url_file=str(secret)) + + seen: list[str] = [] + engine = make_engine( + settings.resolved_database_url, + refresh=lambda: (seen.append(settings.resolved_database_url) or settings.resolved_database_url), + ) + with engine.connect(): + pass + assert seen, "the refresh hook must run when the pool opens a connection" + + +def test_pool_recycle_is_shorter_than_the_shortest_lease() -> None: + """30-minute runtime lease; a pooled connection must be retired first.""" + from canned_prompts_service.db import POOL_RECYCLE_SECONDS + + assert POOL_RECYCLE_SECONDS < 30 * 60