diff --git a/CHANGELOG.md b/CHANGELOG.md index 47aea315a..4566e07b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,21 @@ All notable changes to this project are documented in this file. The format is b - Hosting: readiness and setup connection probes have a five-second deadline per check. Timeouts report "timed out"; raised exceptions are logged and return only their class name, including database connection failures (#786). +- Setup migrations lock and migrate the app's resolved database, including in + newly scaffolded hosts, when it differs from `SM_DATABASE_URL` (#782). Hosts + scaffolded earlier keep their own `migrations/env.py`: read + `config.attributes.get(ALEMBIC_DATABASE_URL_ATTRIBUTE)` (from + `simple_module_hosting.migrations`) before falling back to `Settings()`. +- Explicit bootstrap process settings survive the pre-app settings merge, + including production environment, module selection and Redis (#783). +- **Behaviour change** (`hosting`): runtime host settings discover the same + project `.env` as boot settings; a field set there now pins the value over a + stored DB override and refuses admin-UI edits, like a process env var (#793). +- **Behaviour change** (`core`, `hosting`): lease and semaphore acquisition + bounds backend calls by the wait deadline; Redis uses finite socket timeouts + without retries, and connectivity failures return not-acquired or log failed + release without masking body errors (#787); `wait=0` now allows one attempt up + to 1 s. - **Behaviour change** (`auth`, `oidc`): a generic-OIDC browser session created before #750 (no authority marker) is adopted only when the issuer recorded in its `oidc_identity` matches the discovered issuer (#764). A session from a diff --git a/docs/framework/i18n.md b/docs/framework/i18n.md index 5bf07f0f5..b8e9a90b1 100644 --- a/docs/framework/i18n.md +++ b/docs/framework/i18n.md @@ -214,7 +214,7 @@ A pydantic validator enforces that the default locale is in the supported list. `HostSettings` is consumed through **two different objects**, and i18n uses the env-backed one: - `Settings` (`HostSettings` + `BootstrapSettings`) inherits `env_prefix="SM_"`, so every field on it resolves from `SM_*` at boot. This instance lands on `app.state.sm.settings`, and it is what `LocaleMiddleware`, the i18n manifest, the shared-props builder and `i18n_deps` all read. -- `app.state.host.settings` is a separate `HostSettings` hydrated from the DB. `HostSettings` on its own declares no `env_prefix`, so this instance is pydantic defaults plus stored overrides. `maintenance_mode` is read from here, which is what makes that flag live-editable. +- `app.state.host.settings` is a separate `HostSettings` hydrated from the DB. It resolves env (process variables, then the project `.env`) → stored overrides → defaults, so an admin-UI edit lands here unless env pins the field. `maintenance_mode` is read from here, which is what makes that flag live-editable. The practical consequence: **changing the i18n values in the admin UI does not change locale resolution.** `LocaleMiddleware` captured its locale set at construction from the env-derived object, and a settings save swaps `app.state.host.settings`, not `app.state.sm.settings`. Moving the served locale set means setting the env var and restarting. The same is true of `multi_tenant` / `tenant_header` — `SM_MULTI_TENANT` is what decides whether `TenantMiddleware` is installed at all, and no DB write can install a middleware after boot. diff --git a/docs/framework/leases.md b/docs/framework/leases.md index 7ca10b6b1..c8e27737b 100644 --- a/docs/framework/leases.md +++ b/docs/framework/leases.md @@ -53,7 +53,18 @@ The backend is process-global because a Celery worker has no `app.state`. `creat | a database, no Redis | `DbLeaseBackend` (`simple_module_db`) | Rows in `sm_lease (name, slot, holder, fence, expires_at)`, primary key `(name, slot)`. Every comparison uses the database clock, so clock skew between hosts does not matter; on Postgres the clock is read after the slot row is locked, so a lock wait never shortens a TTL. | | neither | `InProcessLeaseBackend` | One process only. Also what tests and eager Celery get when nothing else is configured. | -Backend errors propagate. A lease that the backend could not confirm is never reported as held. This is unlike the rate limiter, which falls back to per-process counters. +Acquisition timeouts, and Redis connection failures, log a WARNING and return +`False` (other backends' errors, such as a database outage under +`DbLeaseBackend`, still propagate); `async with` consequently raises `LeaseNotAcquiredError`. An explicit +`wait` bounds backend calls as well as polling, with a 1-second minimum attempt +budget (including `wait=0`). Redis connections and socket reads each have a +2-second timeout and at most one immediate reconnect (so roughly 4 s at +worst against an unresponsive server), which also bounds calls when no +`wait` is supplied. A Redis release timeout or connection failure logs a +WARNING and returns `False`, so cleanup preserves an exception from the lease +body; the remote holder expires by TTL. Refresh errors and other backend +errors still propagate. There is no fallback to a local lease: a lease that +the backend could not confirm is never reported as held. Override the backend with `configure_leases(backend)`, or pass `backend=` to a single `lease()`/`semaphore()` call. `configure_leases(None)` restores a fresh in-process backend, which is useful in test teardown. diff --git a/docs/framework/settings.md b/docs/framework/settings.md index 23a601293..20e375793 100644 --- a/docs/framework/settings.md +++ b/docs/framework/settings.md @@ -29,6 +29,21 @@ class BootstrapSettings(BaseSettings): The `SM_` prefix means every field is set via `SM_` (e.g. `SM_DATABASE_URL`, `SM_LOG_FORMAT`). `multi_tenant` / `tenant_header` and the other tunables live on the DB-backed `HostSettings` half. +Hosts call `merge_host_settings()` before `create_app` to apply stored host +settings before middleware and module discovery are built. Precedence is +**env → DB → default**. Here, env includes the discovered project `.env`; +process environment variables take precedence over that file. Both the boot +`Settings` and runtime `HostSettings` discover the same file via +`find_env_file()`, including from a project subdirectory, without requiring +`load_dotenv_into_environ()` first. Direct settings construction supports an +explicit `_env_file` path or `_env_file=None` to disable dotenv loading. + +When passing a `BootstrapSettings` to `merge_host_settings(bootstrap)`, every +field explicitly supplied on that object is preserved, including `environment`, +`modules_enabled`, `redis_url`, `debug` and `vite_dev_url`. The resolved secret +key and database URL are preserved too. Fields the bootstrap did not supply +still use env → DB → default. + Construct once at boot, put on `app.state.sm.settings`, never mutated. ### Placeholder-secret check diff --git a/docs/reference/env-vars.md b/docs/reference/env-vars.md index e2a2b7b20..934adfa2f 100644 --- a/docs/reference/env-vars.md +++ b/docs/reference/env-vars.md @@ -53,26 +53,27 @@ Pools are **per process**. With multiple `uvicorn --workers`, total connections ## Host settings (`HostSettings`) -These are declared on `HostSettings` and registered under `package="host"`, so they appear in the admin UI at `/admin/settings/`. They are **also** readable from env: `Settings` combines `HostSettings` with `BootstrapSettings` and inherits its `env_prefix="SM_"`, so each field below resolves from the matching `SM_*` variable at boot. - -Which source actually wins depends on the field, because `HostSettings` is consumed through two different objects: - -| Setting | Env var | Default | Read from | Notes | -|---|---|---|---|---| -| `multi_tenant` | `SM_MULTI_TENANT` | `false` | **env at boot** | Decides whether `TenantMiddleware` is installed. A DB write cannot install a middleware after boot. | -| `tenant_header` | `SM_TENANT_HEADER` | `""` | **env at boot** | Header identifying the tenant (empty = header lookup disabled). | -| `i18n_default_locale` | `SM_I18N_DEFAULT_LOCALE` | `en` | **env at boot** | Must be in `i18n_supported_locales`. | -| `i18n_supported_locales` | `SM_I18N_SUPPORTED_LOCALES` | `["en"]` | **env at boot** | JSON array, e.g. `["en","es"]`. | -| `i18n_cookie_name` | `SM_I18N_COOKIE_NAME` | `locale` | **env at boot** | Cookie storing the selected locale. | -| `maintenance_mode` | *(none — see below)* | `false` | **DB at request time** | Serve everyone but admins a 503. DB-backed so flipping it needs no redeploy — see [Deployment](/reference/deployment#maintenance-mode). | -| `maintenance_message` | *(none — see below)* | `""` | **DB at request time** | Optional operator note on the maintenance page. | -| `public_base_url` | `SM_PUBLIC_BASE_URL` | `""` | **DB at request time** | Public origin (`https://example.org`) for canonical / `og:url` in the server-rendered head. Never taken from the request `Host`. See [Inertia](/frontend/inertia#seo-head). | - -The split is not arbitrary. The boot instance (`Settings()`, env-derived) lands on `app.state.sm.settings` and is what configures middleware at construction — `LocaleMiddleware` captures its locale set there, and the tenancy flag decides whether `TenantMiddleware` is added at all. The DB-hydrated instance is a plain `HostSettings` on `app.state.host.settings`, which declares no `env_prefix` and so is defaults-plus-overrides; `MaintenanceMiddleware` reads that one on every request. - -So editing the tenancy or i18n rows in the admin UI updates what the UI shows without moving what the app serves — those need the env var and a restart. Maintenance mode is the one that genuinely takes effect live. - -The two maintenance fields have **no working env var**, and this is worth stating plainly because the shape of the code suggests otherwise. `SM_MAINTENANCE_MODE=true` does set `maintenance_mode` on the boot `Settings()` object — but nothing reads maintenance from there, and the DB-hydrated `HostSettings` that `MaintenanceMiddleware` does read carries no `SM_` prefix, so it never sees the variable. Setting it looks plausible and silently does nothing. Flip the flag in the admin UI, or write the settings row directly. +These are declared on `HostSettings` and registered under `package="host"`, so +they appear in the admin UI at `/admin/settings/`. Both boot `Settings` and +runtime `HostSettings` resolve **env → DB → default**, where env includes the +discovered project `.env` and process variables take precedence over that file. + +| Setting | Env var | Default | Applied | +|---|---|---|---| +| `multi_tenant` | `SM_MULTI_TENANT` | `false` | At boot; restart required. | +| `tenant_header` | `SM_TENANT_HEADER` | `""` | At boot; restart required. | +| `i18n_default_locale` | `SM_I18N_DEFAULT_LOCALE` | `en` | At boot; must be in supported locales. Restart required. | +| `i18n_supported_locales` | `SM_I18N_SUPPORTED_LOCALES` | `["en"]` | At boot; restart required. | +| `i18n_cookie_name` | `SM_I18N_COOKIE_NAME` | `locale` | At boot; restart required. | +| `maintenance_mode` | `SM_MAINTENANCE_MODE` | `false` | Runtime; serve everyone but admins a 503. | +| `maintenance_message` | `SM_MAINTENANCE_MESSAGE` | `""` | Runtime; optional operator note. | +| `public_base_url` | `SM_PUBLIC_BASE_URL` | `""` | Runtime; public origin for canonical / `og:url`. Never derived from request `Host`. | + +`merge_host_settings()` reads stored values before constructing middleware and +registries on `app.state.sm.settings`. Runtime hydration populates +`app.state.host.settings` with the same source precedence. Editing a boot-only +field in the DB takes effect after restart; runtime fields can change live +unless env pins them. See [Settings](/framework/settings). ## Users module @@ -135,8 +136,9 @@ In production (`SM_ENVIRONMENT` not in `{development, testing}`), boot fails if The app loads `.env` via pydantic's `BaseSettings`. Order of precedence: 1. Actual environment variables. -2. `.env` file in the current working directory. -3. Defaults from the `Settings` class. +2. The project `.env` discovered by walking up from the current directory (or under `SM_PROJECT_ROOT`). +3. Stored DB overrides for host settings, when merged or hydrated. +4. Defaults from the settings class. Keep secrets out of `.env.example` — check in only non-secret defaults. diff --git a/framework/cli/simple_module_cli/templates/host/migrations/env.py b/framework/cli/simple_module_cli/templates/host/migrations/env.py index 96ff1db82..90aeb2bcb 100644 --- a/framework/cli/simple_module_cli/templates/host/migrations/env.py +++ b/framework/cli/simple_module_cli/templates/host/migrations/env.py @@ -20,6 +20,7 @@ make_process_revision_directives, render_item, ) +from simple_module_hosting.migrations import ALEMBIC_DATABASE_URL_ATTRIBUTE from simple_module_hosting.settings import Settings from sqlalchemy import engine_from_config, pool from sqlalchemy.engine import make_url @@ -51,8 +52,8 @@ def _get_url() -> str: (a stray app.db, nothing else) that printing the target, password masked, is worth the one line. """ - settings = Settings() - url = settings.database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") + database_url = config.attributes.get(ALEMBIC_DATABASE_URL_ATTRIBUTE) or Settings().database_url + url = database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") logger.info( "Migrating %s (cwd=%s)", make_url(url).render_as_string(hide_password=True), Path.cwd() ) diff --git a/framework/core/simple_module_core/leases.py b/framework/core/simple_module_core/leases.py index bb97752d3..decae2966 100644 --- a/framework/core/simple_module_core/leases.py +++ b/framework/core/simple_module_core/leases.py @@ -20,13 +20,14 @@ installs one with :func:`configure_leases` when the app is built and when a worker boots — Redis when ``SM_REDIS_URL`` is set, else the ``sm_lease`` table, else :class:`InProcessLeaseBackend`, which only coordinates one process. -Backend errors propagate: a lease that cannot be confirmed is never reported -as held. +Acquire timeouts and connectivity failures log a warning and return False. +Other backend errors propagate; an unconfirmed lease is never reported as held. """ from __future__ import annotations import asyncio +import logging import random import threading import time @@ -50,6 +51,10 @@ # Seconds between attempts while waiting for a held lease. _POLL = 0.05 _POLL_MAX = 0.5 +# Shortest deadline one acquire attempt gets, even when the wait has run out: +# ``wait=0`` means "don't queue", not "fail a healthy backend that takes 60 ms". +_ATTEMPT_FLOOR = 1.0 +logger = logging.getLogger(__name__) class LeaseNotAcquiredError(RuntimeError): @@ -62,7 +67,8 @@ class LeaseBackend(Protocol): ``acquire``/``semaphore_acquire`` return the new fence number, or ``None`` when the lease (or every slot) is held by someone else. ``refresh`` and ``release`` return ``False`` when *token* no longer holds it — expired, - released, or taken by another owner. + released, or taken by another owner. Connectivity failures during acquire + should raise built-in ConnectionError; release may log and return False. """ async def acquire(self, name: str, token: str, ttl: float) -> int | None: ... @@ -216,7 +222,14 @@ async def acquire(self, *, blocking: bool | None = None, wait: float | None = No deadline = None if wait is None else time.monotonic() + wait delay = _POLL while True: - fence = await self._try() + remaining = ( + None if deadline is None else max(_ATTEMPT_FLOOR, deadline - time.monotonic()) + ) + try: + fence = await asyncio.wait_for(self._try(), remaining) + except (TimeoutError, ConnectionError): + logger.warning("Lease %r acquire timed out or backend unavailable", self.name) + return False if fence is not None: self.fence = fence return True diff --git a/framework/hosting/simple_module_hosting/_lease_redis.py b/framework/hosting/simple_module_hosting/_lease_redis.py index 7f98a1c9e..211592db2 100644 --- a/framework/hosting/simple_module_hosting/_lease_redis.py +++ b/framework/hosting/simple_module_hosting/_lease_redis.py @@ -8,16 +8,21 @@ members whose score has passed, then admits while ``ZCARD < slots``. * ``INCR :fence`` hands out the fencing token, inside the same script. -Unlike the rate limiter there is no per-process fallback: a lease that Redis -could not confirm must not be reported as held, so errors propagate. +There is no per-process fallback: unavailable Redis cannot confirm a lease. +Acquire connectivity errors become ConnectionError for the lease handle; +release connectivity errors are logged and return False. Refresh errors propagate. """ from __future__ import annotations import asyncio +import logging import os from typing import Any +logger = logging.getLogger(__name__) +_SOCKET_TIMEOUT = 2.0 + # Holder and fence-counter keys live under disjoint prefixes: a name is never # parsed, so ``foo:fence`` cannot collide with the counter of ``foo``. _HOLDER_PREFIX = "sm:lease:h:" @@ -113,13 +118,33 @@ def _redis(self) -> Any: self._client = self._factory() else: import redis.asyncio as aioredis - - self._client = aioredis.from_url(self._url) + from redis.asyncio.retry import Retry + from redis.backoff import NoBackoff + + self._client = aioredis.from_url( + self._url, + socket_timeout=_SOCKET_TIMEOUT, + socket_connect_timeout=_SOCKET_TIMEOUT, + # One reconnect, no backoff: a stale pooled connection gets a fresh + # socket, while the 2 s timeouts keep the worst case bounded. + retry=Retry(NoBackoff(), 1), + ) self._owner = owner return self._client async def _eval(self, script: str, keys: list[str], *args: Any) -> int: - return int(await self._redis().eval(script, len(keys), *keys, *args)) + from redis.exceptions import ConnectionError as RedisConnectionError + from redis.exceptions import TimeoutError as RedisTimeoutError + + try: + return int(await self._redis().eval(script, len(keys), *keys, *args)) + except (RedisConnectionError, RedisTimeoutError) as exc: + if script in (_ACQUIRE, _SEM_ACQUIRE): + raise ConnectionError("Redis lease acquire unavailable") from exc + if script not in (_RELEASE, _SEM_RELEASE): + raise + logger.warning("Redis lease release unavailable (%s)", type(exc).__name__) + return 0 @staticmethod def _lease_keys(name: str) -> list[str]: diff --git a/framework/hosting/simple_module_hosting/_preapp_config.py b/framework/hosting/simple_module_hosting/_preapp_config.py index 8133d0594..de3586d57 100644 --- a/framework/hosting/simple_module_hosting/_preapp_config.py +++ b/framework/hosting/simple_module_hosting/_preapp_config.py @@ -228,7 +228,8 @@ def merge_host_settings( # relative sqlite paths against the discovered project root, and # reconstructing Settings from env alone would redo that resolution # against whatever the cwd happens to be. - fixed = {"secret_key": secret_key, "database_url": bootstrap.database_url} + fixed = bootstrap.model_dump(include=bootstrap.model_fields_set) + fixed.update(secret_key=secret_key, database_url=bootstrap.database_url) # Lenient, like the lifespan hydration: a stored value that parses but no # longer validates (a Literal narrowed in an upgrade) falls back to its # default with a WARNING instead of taking down every worker — and with diff --git a/framework/hosting/simple_module_hosting/host_settings.py b/framework/hosting/simple_module_hosting/host_settings.py index e1c83e20d..ab95c9a2e 100644 --- a/framework/hosting/simple_module_hosting/host_settings.py +++ b/framework/hosting/simple_module_hosting/host_settings.py @@ -13,11 +13,12 @@ from __future__ import annotations -from typing import Literal +from typing import Any, Literal from pydantic import Field, field_validator, model_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic_settings import BaseSettings, DotEnvSettingsSource, SettingsConfigDict from simple_module_core.discovery import DEFAULT_AUTH_PROVIDER +from simple_module_core.dotenv import find_env_file from simple_module_hosting import _host_settings_support as v from simple_module_hosting._api_version import default_api_version as _default_api_version @@ -43,6 +44,16 @@ class HostSettings(BaseSettings): # that has nothing to do with this app. model_config = SettingsConfigDict(env_prefix="SM_", extra="ignore") + def __init__(self, **values: Any) -> None: + if "_env_file" not in values: + values["_env_file"] = find_env_file() + super().__init__(**values) + + @classmethod + def env_seeded_fields(cls) -> set[str]: + """Let hydration rank discovered dotenv values above DB overrides too.""" + return set(DotEnvSettingsSource(cls, env_file=find_env_file())()) + multi_tenant: bool = Field(default=False, json_schema_extra=_RESTART_TENANCY) tenant_header: str = Field(default="", json_schema_extra=_RESTART_TENANCY) default_tenant: str = Field(default="", json_schema_extra=_RESTART_TENANCY) diff --git a/framework/hosting/simple_module_hosting/migrations.py b/framework/hosting/simple_module_hosting/migrations.py index 0be51fb93..7a5f1d626 100644 --- a/framework/hosting/simple_module_hosting/migrations.py +++ b/framework/hosting/simple_module_hosting/migrations.py @@ -10,6 +10,11 @@ _ALEMBIC_INI_RELATIVE = "host/alembic.ini" +#: ``alembic.config.Config.attributes`` key carrying the database URL to migrate. +#: The setup wizard sets it to the app's own engine URL, and ``env.py`` prefers +#: it over ``Settings()`` so the migrated database is the one the lock covers. +ALEMBIC_DATABASE_URL_ATTRIBUTE = "sm.database_url" + def default_alembic_ini() -> str: """Locate ``alembic.ini`` without depending on the process cwd. diff --git a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py index dfba155b8..3d3b8ea3a 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -57,7 +57,11 @@ async def apply_migrations(request: Request, _data: dict) -> dict: from alembic import command from alembic.config import Config as AlembicConfig - from simple_module_hosting.migrations import default_alembic_ini, migration_status + from simple_module_hosting.migrations import ( + ALEMBIC_DATABASE_URL_ATTRIBUTE, + default_alembic_ini, + migration_status, + ) # Resolved through the hosting helper rather than hardcoded: this runs # inside a request, and a literal "host/alembic.ini" is only correct while @@ -69,8 +73,13 @@ def _upgrade() -> None: # branch_labels, so the history legitimately has several heads and # "head" raises CommandError("Multiple head revisions are present"). # This is what `make migrate` runs. - with migration_lock(request.app.state.sm.db.engine.url), _preserve_logging(): - command.upgrade(AlembicConfig(ini_path), "heads") + url = request.app.state.sm.db.engine.url + config = AlembicConfig(ini_path) + config.attributes[ALEMBIC_DATABASE_URL_ATTRIBUTE] = url.render_as_string( + hide_password=False + ) + with migration_lock(url), _preserve_logging(): + command.upgrade(config, "heads") try: async with _MIGRATION_LOCK: diff --git a/framework/hosting/tests/test_hosting_config_sources.py b/framework/hosting/tests/test_hosting_config_sources.py new file mode 100644 index 000000000..250626c0a --- /dev/null +++ b/framework/hosting/tests/test_hosting_config_sources.py @@ -0,0 +1,85 @@ +"""Bootstrap and runtime settings use consistent sources (#783, #793).""" + +import os +from pathlib import Path + +import pytest +from simple_module_core.settings_hydrate import build_settings, env_supplied_fields +from simple_module_hosting._preapp_config import merge_host_settings +from simple_module_hosting.bootstrap_settings import BootstrapSettings +from simple_module_hosting.host_settings import HostSettings + + +@pytest.fixture +def project(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + for key in os.environ: + if key.startswith("SM_"): + monkeypatch.delenv(key) + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("SM_PROJECT_ROOT", str(tmp_path)) + return tmp_path + + +def test_explicit_bootstrap_fields_survive_merge(project: Path, monkeypatch) -> None: + monkeypatch.setattr( + "simple_module_hosting._preapp_config.load_host_overrides", lambda *a, **k: {} + ) + bootstrap = BootstrapSettings( + _env_file=None, + database_url=f"sqlite+aiosqlite:///{project}/bootstrap.db", + secret_key="k" * 64, + environment="production", + modules_enabled=["Users"], + redis_url="redis://example.org/1", + debug=True, + vite_dev_url="http://localhost:5999", + ) + merged = merge_host_settings(bootstrap, strict=False) + for name in BootstrapSettings.model_fields: + assert getattr(merged, name) == getattr(bootstrap, name), name + + +def test_unset_bootstrap_fields_still_read_env(project: Path, monkeypatch) -> None: + url = f"sqlite+aiosqlite:///{project}/settings.db" + monkeypatch.setattr( + "simple_module_hosting._preapp_config.load_host_overrides", + lambda *a, **k: {"log_level": ("WARNING", "string")}, + ) + bootstrap = BootstrapSettings(_env_file=None, database_url=url, secret_key="k" * 64) + monkeypatch.setenv("SM_DEBUG", "true") + monkeypatch.setenv("SM_LOG_LEVEL", "ERROR") + merged = merge_host_settings(bootstrap) + assert merged.debug is True + assert merged.log_level == "ERROR" + + +@pytest.mark.parametrize("subdirectory", [False, True]) +def test_dotenv_beats_db_at_boot_and_runtime(project: Path, monkeypatch, subdirectory) -> None: + url = f"sqlite+aiosqlite:///{project}/settings.db" + (project / ".env").write_text("SM_PUBLIC_BASE_URL=https://env.example\n") + if subdirectory: + child = project / "child" + child.mkdir() + monkeypatch.chdir(child) + monkeypatch.setattr( + "simple_module_hosting._preapp_config.load_host_overrides", + lambda *a, **k: {"public_base_url": ("https://db.example", "string")}, + ) + boot = merge_host_settings(BootstrapSettings(database_url=url, secret_key="k" * 64)) + runtime = build_settings(HostSettings, {"public_base_url": ("https://db.example", "string")}) + assert boot.public_base_url == runtime.public_base_url == "https://env.example" + assert "public_base_url" in env_supplied_fields(HostSettings) + assert "SM_PUBLIC_BASE_URL" not in os.environ + + +def test_host_settings_explicit_env_file_and_opt_out(project: Path, monkeypatch) -> None: + (project / ".env").write_text("SM_PUBLIC_BASE_URL=https://default.example\n") + custom = project / "custom.env" + custom.write_text("SM_PUBLIC_BASE_URL=https://custom.example\n") + assert HostSettings(_env_file=custom).public_base_url == "https://custom.example" + assert HostSettings(_env_file=None).public_base_url == "" + child = project / "child" + child.mkdir() + monkeypatch.chdir(child) + assert HostSettings(_env_file=".env").public_base_url == "" + assert HostSettings().public_base_url == "https://default.example" diff --git a/framework/hosting/tests/test_hosting_lease_timeouts.py b/framework/hosting/tests/test_hosting_lease_timeouts.py new file mode 100644 index 000000000..12a3b0085 --- /dev/null +++ b/framework/hosting/tests/test_hosting_lease_timeouts.py @@ -0,0 +1,163 @@ +"""Redis outages respect lease deadlines and do not break cleanup (#787).""" + +import asyncio +import time +from unittest.mock import AsyncMock + +import pytest +from redis.exceptions import ConnectionError, TimeoutError +from simple_module_core.leases import _ATTEMPT_FLOOR, LeaseNotAcquiredError, lease, semaphore +from simple_module_hosting._lease_redis import _SOCKET_TIMEOUT, RedisLeaseBackend + + +@pytest.mark.parametrize("slots", [None, 2]) +async def test_unresponsive_redis_respects_wait(slots, caplog) -> None: + writers = [] + + def accept(_reader, writer): + writers.append(writer) + + try: + server = await asyncio.start_server(accept, "127.0.0.1", 0) + except OSError as exc: + pytest.skip(f"Local TCP binding unavailable: {exc}") + port = server.sockets[0].getsockname()[1] + backend = RedisLeaseBackend(f"redis://127.0.0.1:{port}/0") + held = ( + lease("silent", 10, wait=0.1, backend=backend) + if slots is None + else semaphore("silent", slots, 10, wait=0.1, backend=backend) + ) + try: + start = time.monotonic() + assert await held.acquire() is False + # One attempt is bounded by the per-attempt floor (1 s), not the socket retries. + assert time.monotonic() - start < 3 + assert held.acquired is False + assert "WARNING" in caplog.text + finally: + # Close the accepted sockets before ``wait_closed``: since 3.12 it waits + # for every connection the server accepted, so the reverse order hangs. + await backend._redis().aclose() + for writer in writers: + writer.close() + server.close() + await server.wait_closed() + + +@pytest.mark.parametrize("error", [TimeoutError, ConnectionError]) +@pytest.mark.parametrize("slots", [None, 2]) +async def test_redis_errors_fail_acquire_and_preserve_body_error(error, slots, caplog) -> None: + client = AsyncMock() + backend = RedisLeaseBackend("redis://unused", factory=lambda: client) + held = ( + lease("outage", 10, backend=backend) + if slots is None + else semaphore("outage", slots, 10, backend=backend) + ) + client.eval.side_effect = error("unavailable") + assert await held.acquire() is False + with pytest.raises(LeaseNotAcquiredError): + async with held: + pytest.fail("unconfirmed lease entered") + client.eval.side_effect = None + client.eval.return_value = 1 + with pytest.raises(ValueError, match="body failed"): + async with held: + client.eval.side_effect = error("unavailable") + raise ValueError("body failed") + assert held.acquired is False + assert "WARNING" in caplog.text + + +async def test_redis_client_has_finite_timeouts_and_one_reconnect() -> None: + backend = RedisLeaseBackend("redis://localhost") + client = backend._redis() + try: + options = client.connection_pool.connection_kwargs + assert options["socket_timeout"] == _SOCKET_TIMEOUT == 2.0 + assert options["socket_connect_timeout"] == _SOCKET_TIMEOUT + assert options["retry"].get_retries() == 1 + finally: + await client.aclose() + + +@pytest.mark.parametrize("slots", [None, 2]) +async def test_stalled_backend_call_is_cancelled_at_deadline(slots, caplog) -> None: + cancelled = asyncio.Event() + + async def stall(*args): + try: + await asyncio.Event().wait() + finally: + cancelled.set() + + client = AsyncMock() + client.eval.side_effect = stall + backend = RedisLeaseBackend("redis://unused", factory=lambda: client) + held = ( + lease("stalled", 10, wait=0.05, backend=backend) + if slots is None + else semaphore("stalled", slots, 10, wait=0.05, backend=backend) + ) + assert await asyncio.wait_for(held.acquire(), 3.0) is False + assert cancelled.is_set() + assert not held.acquired + assert "WARNING" in caplog.text + + +@pytest.mark.parametrize("wait", [0, 0.1]) +async def test_immediate_acquire_can_succeed_with_zero_wait(wait) -> None: + client = AsyncMock() + client.eval.return_value = 1 + held = lease( + "ready", 10, wait=wait, backend=RedisLeaseBackend("redis://unused", factory=lambda: client) + ) + assert await held.acquire() + assert await held.release() + + +async def test_refresh_errors_and_cancellation_still_propagate() -> None: + client = AsyncMock() + held = lease("refresh", 10, backend=RedisLeaseBackend("redis://unused", factory=lambda: client)) + client.eval.side_effect = ConnectionError("unavailable") + with pytest.raises(ConnectionError): + await held.refresh() + client.eval.side_effect = asyncio.CancelledError() + with pytest.raises(asyncio.CancelledError): + await held.acquire() + + +async def test_zero_wait_tolerates_a_slow_but_healthy_backend() -> None: + """``wait=0`` means "do not queue", so a 200 ms reply still acquires.""" + + async def slow(*_args): + await asyncio.sleep(0.2) # intentional-sleep: simulated slow backend reply + return 1 + + client = AsyncMock() + client.eval.side_effect = slow + held = lease( + "slow", 10, wait=0, backend=RedisLeaseBackend("redis://unused", factory=lambda: client) + ) + assert await held.acquire() + + +async def test_attempt_floor_is_one_second_at_zero_wait() -> None: + """The floor is the documented 1 s: a 0.8 s reply acquires, a stall gives up at ~1 s.""" + assert _ATTEMPT_FLOOR == 1.0 + + delay = 0.8 + + async def reply(*_args): + await asyncio.sleep(delay) # intentional-sleep: simulated backend latency + return 1 + + client = AsyncMock() + client.eval.side_effect = reply + backend = RedisLeaseBackend("redis://unused", factory=lambda: client) + assert await lease("floor-ok", 10, wait=0, backend=backend).acquire() + delay = 30 + start = time.monotonic() + assert await lease("floor-stall", 10, wait=0, backend=backend).acquire() is False + assert 0.9 <= time.monotonic() - start < 2.5 diff --git a/framework/hosting/tests/test_hosting_migration_target.py b/framework/hosting/tests/test_hosting_migration_target.py new file mode 100644 index 000000000..b7cbc5d7d --- /dev/null +++ b/framework/hosting/tests/test_hosting_migration_target.py @@ -0,0 +1,78 @@ +"""The wizard locks and migrates the app DB; CLI retains env fallback (#782).""" + +import asyncio +import contextlib +import sqlite3 +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest +from alembic import command +from alembic.config import Config +from simple_module_hosting.setup_wizard import migrate +from sqlalchemy.ext.asyncio import create_async_engine + +_ROOT = Path(__file__).resolve().parents[3] +_ENVS = [ + _ROOT / "host/migrations/env.py", + _ROOT / "framework/cli/simple_module_cli/templates/host/migrations/env.py", +] + + +@pytest.mark.parametrize("env_path", _ENVS, ids=["host", "scaffold"]) +@pytest.mark.parametrize("wizard", [True, False], ids=["wizard", "cli"]) +async def test_migration_target(tmp_path, monkeypatch, env_path, wizard) -> None: + app_db, env_db = tmp_path / "app.db", tmp_path / "env.db" + monkeypatch.setenv("SM_DATABASE_URL", f"sqlite+aiosqlite:///{env_db}") + scripts = tmp_path / "migrations" + (scripts / "versions").mkdir(parents=True) + (scripts / "env.py").write_text(env_path.read_text()) + (scripts / "versions/initial.py").write_text( + "from alembic import op\nimport sqlalchemy as sa\n" + 'revision = "initial"\ndown_revision = None\n' + 'def upgrade():\n op.create_table("marker", sa.Column("id", sa.Integer))\n' + 'def downgrade():\n op.drop_table("marker")\n' + ) + ini = tmp_path / "alembic.ini" + ini.write_text( + f"[alembic]\nscript_location = {scripts}\n" + "[loggers]\nkeys=root\n[handlers]\nkeys=\n[formatters]\nkeys=\n" + "[logger_root]\nlevel=WARNING\nhandlers=\n" + ) + locked = [] + + async def inline_thread(func, *args): + return func(*args) + + monkeypatch.setattr(asyncio, "to_thread", inline_thread) + + @contextlib.contextmanager + def lock(url): + locked.append(url) + yield + + monkeypatch.setattr(migrate, "migration_lock", lock) + monkeypatch.setattr(migrate, "run_deferred_startup", AsyncMock()) + monkeypatch.setattr("simple_module_hosting.migrations.default_alembic_ini", lambda: str(ini)) + monkeypatch.setattr("simple_module_hosting.migrations.migration_status", AsyncMock()) + engine = create_async_engine(f"sqlite+aiosqlite:///{app_db}") + request = SimpleNamespace( + app=SimpleNamespace( + state=SimpleNamespace(sm=SimpleNamespace(db=SimpleNamespace(engine=engine))) + ), + state=SimpleNamespace(), + ) + try: + if wizard: + await migrate.apply_migrations(request, {}) + assert locked == [engine.url] + else: + await asyncio.to_thread(command.upgrade, Config(str(ini)), "heads") + for path, migrated in [(app_db, wizard), (env_db, not wizard)]: + with contextlib.closing(sqlite3.connect(path)) as db: + tables = {row[0] for row in db.execute("SELECT name FROM sqlite_master")} + assert ("alembic_version" in tables) is migrated + assert ("marker" in tables) is migrated + finally: + await engine.dispose() diff --git a/host/migrations/env.py b/host/migrations/env.py index 3b86b7cd3..80a2da1b0 100644 --- a/host/migrations/env.py +++ b/host/migrations/env.py @@ -20,6 +20,7 @@ make_process_revision_directives, render_item, ) +from simple_module_hosting.migrations import ALEMBIC_DATABASE_URL_ATTRIBUTE from simple_module_hosting.settings import Settings from sqlalchemy import engine_from_config, pool from sqlalchemy.engine import make_url @@ -58,8 +59,8 @@ def _get_url() -> str: (a stray app.db, nothing else) that printing the target, password masked, is worth the one line. """ - settings = Settings() - url = settings.database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") + database_url = config.attributes.get(ALEMBIC_DATABASE_URL_ATTRIBUTE) or Settings().database_url + url = database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") logger.info( "Migrating %s (cwd=%s)", make_url(url).render_as_string(hide_password=True), Path.cwd() )