From db25256af7a3cb682950e048be843e9dd7ce4794 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 11:45:32 +0200 Subject: [PATCH 1/9] hosting: one is_public() for every public-route check; legacy provider failures are logged Closes #593 Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- ...26-05-27-pluggable-auth-keycloak-design.md | 4 ++ .../specs/hifi-gap/auth-screens-02-07.md | 4 ++ .../simple_module_hosting/_rate_limit.py | 9 +-- .../simple_module_hosting/_route_access.py | 17 +---- .../simple_module_hosting/maintenance.py | 26 +------ .../simple_module_hosting/public_access.py | 26 +++++++ .../tests/test_hosting_public_access.py | 67 +++++++++++++++++++ modules/auth/auth/middleware.py | 13 +--- 8 files changed, 112 insertions(+), 54 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/public_access.py create mode 100644 framework/hosting/tests/test_hosting_public_access.py diff --git a/docs/superpowers/specs/2026-05-27-pluggable-auth-keycloak-design.md b/docs/superpowers/specs/2026-05-27-pluggable-auth-keycloak-design.md index 3b34805f5..fa728f202 100644 --- a/docs/superpowers/specs/2026-05-27-pluggable-auth-keycloak-design.md +++ b/docs/superpowers/specs/2026-05-27-pluggable-auth-keycloak-design.md @@ -1,5 +1,9 @@ # Pluggable Auth: AuthProvider Contract + Keycloak Module +> **Superseded:** Guidance below to implement `get_public_paths()` is obsolete. Use +> `ModuleBase.register_public_routes` and the method-aware `PublicRouteRegistry`; +> see [Public routes](../../framework/public-routes.md). The legacy shim is removed in 0.1.0. + **Date:** 2026-05-27 **Status:** Design — approved for implementation planning **Builds on:** [2026-05-21 Auth principal-resolver chain](2026-05-21-auth-principal-resolver-design.md) (must land first) diff --git a/docs/superpowers/specs/hifi-gap/auth-screens-02-07.md b/docs/superpowers/specs/hifi-gap/auth-screens-02-07.md index d4d8085ab..fc1e9e1b9 100644 --- a/docs/superpowers/specs/hifi-gap/auth-screens-02-07.md +++ b/docs/superpowers/specs/hifi-gap/auth-screens-02-07.md @@ -1,5 +1,9 @@ # Hi-Fi deck gap analysis — auth-screens-02-07 +> **Superseded:** Guidance below to implement `get_public_paths()` is obsolete. Use +> `ModuleBase.register_public_routes` and the method-aware `PublicRouteRegistry`; +> see [Public routes](../../../framework/public-routes.md). The legacy shim is removed in 0.1.0. + Generated 2026-09-02 from the cached deck (fetched 2026-08-19) vs main @ a8ab6bb. Read-only findings; decisions live in ../2026-09-03-hifi-pages-design.md. # Design-vs-implementation gap analysis: auth screens (02–07) diff --git a/framework/hosting/simple_module_hosting/_rate_limit.py b/framework/hosting/simple_module_hosting/_rate_limit.py index 90bc959af..48c1bbb89 100644 --- a/framework/hosting/simple_module_hosting/_rate_limit.py +++ b/framework/hosting/simple_module_hosting/_rate_limit.py @@ -11,8 +11,8 @@ (``_rate_limit_keys``); the tiers below still apply afterwards; * a request is limited by ``rate_limit_public`` only when the auth middleware judged it anonymous-allowed (``scope["state"]["auth_public"]``: framework defaults, the - registry and the provider legacy paths; the registry alone if no auth provider - is installed) **and** nobody is signed in. Health/static are never limited; + registry and the provider legacy paths; the shared public-access check if no auth middleware + records a decision) **and** nobody is signed in. Health/static are never limited; * a matching rule's own ``rate=`` replaces that default for the rule; * ``rate_limit_authenticated`` (off by default) limits signed-in traffic, one budget per user (anonymous-or-id-less principals fall back to their IP); @@ -38,6 +38,7 @@ from simple_module_hosting._rate_limit_keys import KEY_IP, KEY_USER_OR_IP, rule_bucket, rule_key from simple_module_hosting._request_guard_responses import send_guard_response +from simple_module_hosting.public_access import is_public __all__ = ["RateLimitMiddleware"] @@ -70,8 +71,8 @@ def _policy(self, scope: Scope) -> tuple[str, RateSpec, str] | None: registry = getattr(scope["app"].state, "public_routes", None) rule = registry.match(scope["method"], scope["path"]) if registry is not None else None flag = scope.get("state", {}).get("auth_public") - if flag is None: # no auth provider recorded a decision: registry is the truth - flag = rule is not None + if flag is None: # no auth middleware recorded a decision + flag = is_public(scope["app"], scope["method"], scope["path"]) if not flag: return None if rule is None and scope["path"].startswith(_OPERATIONAL_PREFIXES): diff --git a/framework/hosting/simple_module_hosting/_route_access.py b/framework/hosting/simple_module_hosting/_route_access.py index 1a5ceb4d1..b48167d44 100644 --- a/framework/hosting/simple_module_hosting/_route_access.py +++ b/framework/hosting/simple_module_hosting/_route_access.py @@ -23,6 +23,7 @@ from fastapi.routing import iter_route_contexts from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel +from simple_module_hosting.public_access import is_public from simple_module_hosting.token_guard import ( GATE_ALLOW, GATE_AUTHENTICATED, @@ -63,27 +64,13 @@ def iter_api_operations(app: FastAPI) -> Iterator[ApiOperation]: yield ApiOperation(path, methods, frozenset(dependant_gate_kinds(dependant))) -def _legacy_public(app: FastAPI, path: str) -> bool: - provider = getattr(getattr(app.state, "auth", None), "auth_provider", None) - if provider is None: - return False - try: - prefixes, exact = provider.get_public_paths() - except Exception: - return False - return any(path.startswith(p) for p in prefixes) or path in exact - - def is_public_operation(app: FastAPI, method: str, path: str) -> bool: """Whether an anonymous *method* on the templated *path* is let through.""" if path == getattr(app.state, "openapi_url", None): return True - registry = getattr(app.state, "public_routes", None) for placeholder in _PLACEHOLDERS: concrete = _PARAM.sub(placeholder, path) - if registry is not None and registry.matches(method, concrete): - return True - if _legacy_public(app, concrete): + if is_public(app, method, concrete): return True return False diff --git a/framework/hosting/simple_module_hosting/maintenance.py b/framework/hosting/simple_module_hosting/maintenance.py index bd7e7c6e5..8de2ad134 100644 --- a/framework/hosting/simple_module_hosting/maintenance.py +++ b/framework/hosting/simple_module_hosting/maintenance.py @@ -12,15 +12,12 @@ from __future__ import annotations -import logging - from simple_module_core.permissions import is_admin_principal -from simple_module_core.public_routes import legacy_provider_public_paths from starlette.requests import Request from starlette.responses import JSONResponse from starlette.types import ASGIApp, Receive, Scope, Send -logger = logging.getLogger(__name__) +from simple_module_hosting.public_access import is_public # Kept reachable while the gate is closed: liveness probes (so orchestrators # do not kill the pod mid-maintenance), static assets and i18n bundles (or the @@ -89,26 +86,7 @@ def _may_bypass(request: Request, scope: Scope) -> bool: path: str = scope["path"] - # Module-contributed public routes (register_public_routes hook) — the - # same registry AuthMiddleware consults. Without this, a route a module - # deliberately exempted from auth (branding's logo/favicon, a webhook, - # a STAC/OGC read endpoint) still 503s during maintenance. - public_routes = getattr(scope["app"].state, "public_routes", None) - if public_routes is not None and public_routes.matches(request.method, path): - return True - - # An admin locked out by the switch still needs the login flow. Ask the - # provider which paths those are rather than hardcoding a module's URLs. - auth_state = getattr(scope["app"].state, "auth", None) - provider = getattr(auth_state, "auth_provider", None) - if provider is None: - return False - try: - prefix_paths, exact_paths = legacy_provider_public_paths(provider) - except Exception: - logger.exception("Auth provider failed to report public paths") - return False - return any(path.startswith(p) for p in prefix_paths) or path in exact_paths + return is_public(scope["app"], request.method, path) @staticmethod async def _render(request: Request, message: str): diff --git a/framework/hosting/simple_module_hosting/public_access.py b/framework/hosting/simple_module_hosting/public_access.py new file mode 100644 index 000000000..4aee11d58 --- /dev/null +++ b/framework/hosting/simple_module_hosting/public_access.py @@ -0,0 +1,26 @@ +"""Shared public access, including the pre-0.1.0 provider shim.""" + +from __future__ import annotations + +import logging + +from fastapi import FastAPI +from simple_module_core.public_routes import legacy_provider_public_paths + +logger = logging.getLogger(__name__) + + +def is_public(app: FastAPI, method: str, path: str) -> bool: + """Whether the registry or legacy provider opens this concrete path.""" + registry = getattr(app.state, "public_routes", None) + if registry is not None and registry.matches(method, path): + return True + provider = getattr(getattr(app.state, "auth", None), "auth_provider", None) + if provider is None: + return False + try: + prefixes, exact = legacy_provider_public_paths(provider) + except Exception: + logger.exception("Auth provider failed to report public paths") + return False + return any(path.startswith(prefix) for prefix in prefixes) or path in exact diff --git a/framework/hosting/tests/test_hosting_public_access.py b/framework/hosting/tests/test_hosting_public_access.py new file mode 100644 index 000000000..41505f078 --- /dev/null +++ b/framework/hosting/tests/test_hosting_public_access.py @@ -0,0 +1,67 @@ +"""Shared public-route decisions and legacy-provider failure reporting.""" + +from types import SimpleNamespace + +import pytest +from fastapi import FastAPI +from simple_module_core.public_routes import PublicRouteRegistry +from simple_module_hosting._route_access import is_public_operation +from simple_module_hosting.public_access import is_public + + +def test_registry_is_method_aware_and_skips_provider() -> None: + class UnexpectedProvider: + def get_public_paths(self): + pytest.fail("Registry hits must not consult the legacy provider") + + app = FastAPI() + registry = PublicRouteRegistry() + registry.add_exact("/api/public", methods={"GET"}) + app.state.public_routes = registry + app.state.auth = SimpleNamespace(auth_provider=UnexpectedProvider()) + assert is_public(app, "GET", "/api/public") + app.state.auth.auth_provider = None + assert not is_public(app, "POST", "/api/public") + assert not is_public(app, "GET", "/api/private") + + +def test_legacy_prefix_and_exact_paths_warn_once() -> None: + class LegacyProvider: + def get_public_paths(self): + return ("/legacy/",), ("/login",) + + app = FastAPI() + app.state.auth = SimpleNamespace(auth_provider=LegacyProvider()) + with pytest.warns(DeprecationWarning, match="get_public_paths") as warnings: + assert is_public(app, "GET", "/legacy/item") + assert is_public(app, "POST", "/login") + assert not is_public(app, "GET", "/login-extra") + assert len(warnings) == 1 + + +@pytest.mark.parametrize("operation", [False, True]) +def test_raising_legacy_provider_is_logged(caplog, operation: bool) -> None: + class RaisingProvider: + def get_public_paths(self): + raise RuntimeError("provider failed") + + app = FastAPI() + app.state.auth = SimpleNamespace(auth_provider=RaisingProvider()) + check = is_public_operation if operation else is_public + with pytest.warns(DeprecationWarning, match="get_public_paths"): + assert not check(app, "GET", "/api/private/{id}") + assert "Auth provider failed to report public paths" in caplog.text + assert "provider failed" in caplog.text + + +def test_operation_keeps_placeholder_substitution() -> None: + app = FastAPI() + registry = PublicRouteRegistry() + registry.add_regex(r"/api/items/\d+$", methods={"GET"}) + app.state.public_routes = registry + assert is_public_operation(app, "GET", "/api/items/{id}") + assert not is_public_operation(app, "POST", "/api/items/{id}") + + +def test_missing_registry_and_provider_is_private() -> None: + assert not is_public(FastAPI(), "GET", "/api/private") diff --git a/modules/auth/auth/middleware.py b/modules/auth/auth/middleware.py index 6c84db616..897b4b00e 100644 --- a/modules/auth/auth/middleware.py +++ b/modules/auth/auth/middleware.py @@ -10,9 +10,9 @@ import logging -from simple_module_core.public_routes import legacy_provider_public_paths from simple_module_core.redirect_safety import SESSION_NEXT_KEY, safe_next_or_none from simple_module_db.listeners import current_user_id +from simple_module_hosting.public_access import is_public as route_is_public from starlette.requests import Request from starlette.responses import JSONResponse, RedirectResponse from starlette.types import ASGIApp, Receive, Scope, Send @@ -89,17 +89,8 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: # Anonymous exactly when the host serves it (dev, or openapi_public). or path == getattr(app_state, "openapi_url", None) ) - # Module-contributed public routes (register_public_routes hook). Method - # -aware, so a GET read route can be exempted without opening sibling - # POST/PATCH mutations under the same prefix. if not is_public: - public_routes = getattr(app_state, "public_routes", None) - is_public = public_routes is not None and public_routes.matches(method, path) - # Deprecated provider-declared paths (prefix-only, method-agnostic); honoured - # until 0.1.0 for third-party providers. users/keycloak use the registry. - if not is_public: - prefix_paths, exact_paths = legacy_provider_public_paths(provider) - is_public = any(path.startswith(p) for p in prefix_paths) or path in exact_paths + is_public = route_is_public(scope["app"], method, path) # Contract with the host's RateLimitMiddleware (plain scope key, no import # between them): whether *this* middleware treats the request as anonymous- From 8bc32f7305267c22579351da8fe6fd8e79fbc289 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 11:45:32 +0200 Subject: [PATCH 2/9] framework hygiene: state reset hooks, logged fail-open handlers, dated compat paths, route contract, leaf modules Closes #594 Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/framework-conventions.md | 12 + .../simple_module_core/_i18n_translator.py | 5 + .../core/simple_module_core/invalidation.py | 10 +- framework/core/simple_module_core/leases.py | 6 + framework/core/simple_module_core/mail.py | 6 + framework/core/simple_module_core/menu.py | 7 +- framework/core/simple_module_core/module.py | 7 +- .../core/simple_module_core/module_meta.py | 3 +- .../core/simple_module_core/versioning.py | 9 + framework/core/tests/test_versioning.py | 3 +- framework/db/simple_module_db/listeners.py | 5 + .../simple_module_hosting/_body_limit.py | 2 +- .../_deferred_startup.py | 64 ++++++ .../simple_module_hosting/_error_handlers.py | 208 +----------------- .../_error_negotiation.py | 65 ++++++ .../simple_module_hosting/_error_page.py | 156 +++++++++++++ .../simple_module_hosting/_lifespan.py | 55 +---- .../_request_guard_responses.py | 3 +- .../simple_module_hosting/maintenance.py | 9 +- .../hosting/simple_module_hosting/sentry.py | 13 ++ .../simple_module_hosting/setup_gate.py | 19 +- .../setup_wizard/migrate.py | 4 +- .../tests/test_error_status_coverage.py | 3 +- .../test_hosting_error_handling_policy.py | 72 ++++++ .../tests/test_lifespan_deferred_startup.py | 7 +- .../tests/test_menu_providers_wiring.py | 2 +- .../test_route_guard_fastapi_contract.py | 31 +++ .../testing/simple_module_test/fixtures.py | 3 + framework/testing/simple_module_test/mail.py | 13 +- .../testing/simple_module_test/plugin.py | 3 +- .../simple_module_test/process_state.py | 24 ++ .../tests/test_testing_process_state.py | 93 ++++++++ host/tests/test_deprecation_expiry.py | 11 + 33 files changed, 632 insertions(+), 301 deletions(-) create mode 100644 framework/hosting/simple_module_hosting/_deferred_startup.py create mode 100644 framework/hosting/simple_module_hosting/_error_negotiation.py create mode 100644 framework/hosting/simple_module_hosting/_error_page.py create mode 100644 framework/hosting/tests/test_hosting_error_handling_policy.py create mode 100644 framework/testing/simple_module_test/process_state.py create mode 100644 framework/testing/tests/test_testing_process_state.py diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index 2715b80ed..1703508dc 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -739,3 +739,15 @@ The active locale lands on `request.state.locale`. The `` comp App boot runs `I18nDiagnostics` against every module's declared locale dirs. See codes `SM013`–`SM016` in the table above. Warnings are printed in dev; errors fail the boot in production. `make doctor` runs the same checks, and it runs them whether or not `SM_I18N_SUPPORTED_LOCALES` is set. With the variable unset each namespace is checked against the locale files it actually ships: SM014/SM015 still hold every translation to the default catalog's key set, while SM013 stays quiet, since a locale nothing promised cannot be missing. Declaring the set turns SM013 back on — that is the check that reports a module which never shipped one of the locales the install claims to support. + +## Error handling policy + +Unexpected exceptions retained as fallbacks must log at WARNING or higher with +a traceback (`exc_info=True`). Recurring request/transport failures warn on the +first occurrence per process; menu providers warn once per provider per registry. +Deliberate fail-open sites are audit identity inspection (`db/listeners.py`, +omit the unavailable primary key), menu providers (`core/menu.py`, omit that +provider's items), plural locale parsing (`core/_i18n_translator.py`, use `other`), +and invalidation decoding (`core/invalidation.py`, drop the malformed message +and keep listening). The migration re-check (`hosting/setup_gate.py`) fails +closed: retain the incomplete boot snapshot and keep the setup gate active. diff --git a/framework/core/simple_module_core/_i18n_translator.py b/framework/core/simple_module_core/_i18n_translator.py index 95d434f1e..c9c36745d 100644 --- a/framework/core/simple_module_core/_i18n_translator.py +++ b/framework/core/simple_module_core/_i18n_translator.py @@ -14,6 +14,7 @@ # Logged under the historical ``simple_module_core.i18n`` name so existing log # filters keep matching after the split. logger = logging.getLogger("simple_module_core.i18n") +_plural_failure_warned = False @lru_cache(maxsize=64) @@ -30,6 +31,10 @@ def _plural_form(locale: str, count: float) -> str: try: rule = _plural_rule(locale) except Exception: + global _plural_failure_warned + if not _plural_failure_warned: + _plural_failure_warned = True + logger.warning("Plural locale parsing failed; using other", exc_info=True) return "other" return rule(count) diff --git a/framework/core/simple_module_core/invalidation.py b/framework/core/simple_module_core/invalidation.py index e1bcf712e..582f4110d 100644 --- a/framework/core/simple_module_core/invalidation.py +++ b/framework/core/simple_module_core/invalidation.py @@ -48,6 +48,7 @@ from typing import Protocol, runtime_checkable logger = logging.getLogger(__name__) +_decode_failure_warned = False __all__ = [ "WIRE_VERSION", @@ -108,8 +109,8 @@ def from_wire(cls, message: str | bytes) -> Invalidation | None: routinely shared — with another app, with a stray ``redis-cli publish``, with the previous version of this code — and a listener loop that dies on one malformed payload takes cross-process invalidation down for the - life of the process. Logged at debug: on a shared database this is - normal traffic, not a fault. + life of the process. Decode exceptions warn once per process with a + traceback; unsupported shapes remain ordinary shared-database traffic. """ try: data = json.loads(message) @@ -123,7 +124,10 @@ def from_wire(cls, message: str | bytes) -> Invalidation | None: return None return cls(channel=channel, key=key, origin=str(data.get("origin") or "")) except Exception: - logger.debug("Ignoring undecodable invalidation message: %r", message) + global _decode_failure_warned + if not _decode_failure_warned: + _decode_failure_warned = True + logger.warning("Ignoring undecodable invalidation message", exc_info=True) return None diff --git a/framework/core/simple_module_core/leases.py b/framework/core/simple_module_core/leases.py index e230e6b81..bb97752d3 100644 --- a/framework/core/simple_module_core/leases.py +++ b/framework/core/simple_module_core/leases.py @@ -43,6 +43,7 @@ "configure_leases", "get_leases", "lease", + "reset_lease_backend", "semaphore", ] @@ -153,6 +154,11 @@ def configure_leases(backend: LeaseBackend | None) -> None: _backend = backend if backend is not None else InProcessLeaseBackend() +def reset_lease_backend() -> None: + """Discard process lease configuration and held in-process leases.""" + configure_leases(None) + + def get_leases() -> LeaseBackend: """The process-wide backend :func:`lease` and :func:`semaphore` use.""" return _backend diff --git a/framework/core/simple_module_core/mail.py b/framework/core/simple_module_core/mail.py index b41bf9de5..8579bb49c 100644 --- a/framework/core/simple_module_core/mail.py +++ b/framework/core/simple_module_core/mail.py @@ -93,6 +93,12 @@ def install_mail_resolver(resolver: MailSenderResolver | None) -> None: _resolver = resolver +def reset_mail_sender() -> None: + """Clear the process sender and resolver between isolated app/test runs.""" + install_mail_sender(None) + install_mail_resolver(None) + + def mail_transport_installed() -> bool: """Whether a transport (sender or resolver) is installed in this process. diff --git a/framework/core/simple_module_core/menu.py b/framework/core/simple_module_core/menu.py index 5a6ac85b3..cc0e241fa 100644 --- a/framework/core/simple_module_core/menu.py +++ b/framework/core/simple_module_core/menu.py @@ -85,6 +85,7 @@ def __init__(self) -> None: self._items: list[MenuItem] = [] self._sorted: list[MenuItem] | None = None self._providers: list[MenuProvider] = [] + self._failed_providers: set[int] = set() def _invalidate(self) -> None: self._sorted = None @@ -125,7 +126,11 @@ async def collect_provider_items(self, request: Request) -> list[MenuItem]: # Materialise first: a generator failing midway must add nothing. items.extend(list(result)) except Exception: - logger.exception("Menu provider %r failed; contributing nothing", provider) + if id(provider) not in self._failed_providers: + self._failed_providers.add(id(provider)) + logger.warning( + "Menu provider %r failed; contributing nothing", provider, exc_info=True + ) return items @property diff --git a/framework/core/simple_module_core/module.py b/framework/core/simple_module_core/module.py index 2d020209b..b09a0e2d6 100644 --- a/framework/core/simple_module_core/module.py +++ b/framework/core/simple_module_core/module.py @@ -25,6 +25,9 @@ from simple_module_core.spa_mount import SpaMount +EVENT_HANDLERS_OPTIONAL_APP_REMOVE_IN = "0.1.0" + + class ModuleBase(RequestHooksMixin, ABC): """Base class for all modules. @@ -75,7 +78,9 @@ def register_feature_flags(self, registry: FeatureFlagRegistry) -> None: def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> None: """Subscribe to events published by other modules. - ``app`` is optional for back-compat; pass it through to handlers + The optional ``app=None`` compatibility default has + ``remove_in="0.1.0"``. Overrides own the call, so the base class cannot + reliably warn when callers omit it. Pass ``app`` through to handlers that need ``app.state.sm.db.session_factory`` to persist on the framework's engine instead of building their own. """ diff --git a/framework/core/simple_module_core/module_meta.py b/framework/core/simple_module_core/module_meta.py index 6ff9d8701..94ab87fe7 100644 --- a/framework/core/simple_module_core/module_meta.py +++ b/framework/core/simple_module_core/module_meta.py @@ -50,7 +50,8 @@ class ModuleMeta: Example: ``">=1.0,<2.0"``. When set, the module is rejected at boot if the installed ``simple_module_core.FRAMEWORK_API_VERSION`` does not satisfy it. - When ``None``, no compatibility check is performed (legacy modules). + When ``None``, no compatibility check is performed (legacy modules); + this deprecated bypass has ``remove_in="0.1.0"`` and warns at discovery. """ i18n_audience: str = "public" """Who this module's locale catalog is shipped to: ``"public"`` or ``"admin"``. diff --git a/framework/core/simple_module_core/versioning.py b/framework/core/simple_module_core/versioning.py index b36758113..223b6b3d7 100644 --- a/framework/core/simple_module_core/versioning.py +++ b/framework/core/simple_module_core/versioning.py @@ -8,6 +8,7 @@ from packaging.specifiers import InvalidSpecifier, SpecifierSet from packaging.version import Version +from simple_module_core.deprecation import deprecate from simple_module_core.exceptions import FrameworkVersionError from simple_module_core.module import ModuleBase @@ -28,6 +29,13 @@ _FRAMEWORK_VERSION = Version(FRAMEWORK_API_VERSION) +_UNVERSIONED_MODULE = deprecate( + "ModuleMeta.requires_framework=None compatibility bypass", + remove_in="0.1.0", + use="ModuleMeta(requires_framework=...) with an explicit compatible range", +) + + def check_framework_compatibility( modules: Sequence[ModuleBase], framework_version: str = FRAMEWORK_API_VERSION, @@ -48,6 +56,7 @@ def check_framework_compatibility( for mod in modules: spec_str = mod.meta.requires_framework if spec_str is None: + _UNVERSIONED_MODULE.warn() continue try: spec = SpecifierSet(spec_str) diff --git a/framework/core/tests/test_versioning.py b/framework/core/tests/test_versioning.py index 651819c6a..6aaaa2850 100644 --- a/framework/core/tests/test_versioning.py +++ b/framework/core/tests/test_versioning.py @@ -61,7 +61,8 @@ async def test_check_compat_skips_modules_without_spec(self): class ModLegacy(ModuleBase): meta = ModuleMeta(name="Legacy") - check_framework_compatibility([ModLegacy()]) + with pytest.warns(DeprecationWarning, match=r"requires_framework=None.*0\.1\.0"): + check_framework_compatibility([ModLegacy()]) async def test_check_compat_rejects_malformed_spec(self): """A malformed version specifier raises FrameworkVersionError (not something cryptic).""" diff --git a/framework/db/simple_module_db/listeners.py b/framework/db/simple_module_db/listeners.py index 38b4a8aa0..1543d264b 100644 --- a/framework/db/simple_module_db/listeners.py +++ b/framework/db/simple_module_db/listeners.py @@ -42,6 +42,7 @@ from simple_module_db.tenancy import TenantIsolationError, current_tenant_id logger = logging.getLogger(__name__) +_entity_pk_failure_warned = False _db_logger = logging.getLogger("simple_module.db") # Set by auth middleware on each request @@ -82,6 +83,10 @@ def _entity_pk(obj: object) -> object: return identity[0] return identity except Exception: + global _entity_pk_failure_warned + if not _entity_pk_failure_warned: + _entity_pk_failure_warned = True + logger.warning("Audit identity inspection failed; omitting primary key", exc_info=True) return None diff --git a/framework/hosting/simple_module_hosting/_body_limit.py b/framework/hosting/simple_module_hosting/_body_limit.py index ee266eb2a..ad1d88b61 100644 --- a/framework/hosting/simple_module_hosting/_body_limit.py +++ b/framework/hosting/simple_module_hosting/_body_limit.py @@ -22,7 +22,7 @@ from starlette.requests import Request from starlette.types import ASGIApp, Message, Receive, Scope, Send -from simple_module_hosting._error_handlers import _wants_json +from simple_module_hosting._error_negotiation import _wants_json from simple_module_hosting._request_guard_responses import send_guard_response logger = logging.getLogger("simple_module.request_guard") diff --git a/framework/hosting/simple_module_hosting/_deferred_startup.py b/framework/hosting/simple_module_hosting/_deferred_startup.py new file mode 100644 index 000000000..4f75a76d8 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_deferred_startup.py @@ -0,0 +1,64 @@ +"""Settings hydration and deferred startup replay, independent of setup gates.""" + +from __future__ import annotations + +import asyncio +import logging + +from fastapi import FastAPI +from simple_module_core.module_settings import get_module_settings_registry +from simple_module_db.stored_settings import SessionOverrideSource, settings_table_exists + +from simple_module_hosting._hydrate_step import hydrate_all + +logger = logging.getLogger(__name__) + + +async def hydrate_settings_from_db(app: FastAPI) -> None: + """Merge DB-stored overrides into every registered settings object. + + Reads the settings table directly (``simple_module_db.stored_settings``), + so it works whether or not the ``settings`` plugin is enabled (#500). A + missing table is a fresh install: nothing is stored yet, so there is + nothing to merge. + + Note this is the *second* settings read of a boot. The first happens in + ``_preapp_config`` before ``create_app`` builds anything, because the + module list, i18n registry and middleware stack are all constructed from + settings and cannot be rebuilt from here. + """ + if get_module_settings_registry(app) is None: + return + + async with app.state.sm.db.session_factory() as session: + if not await settings_table_exists(session): + return + await hydrate_all(app, SessionOverrideSource(session), session) + + +async def run_deferred_startup(app: FastAPI) -> None: + """Replay the ``on_startup`` hooks that could not run on an unmigrated DB. + + Called from the wizard's migrations action and from the setup gate's + schema re-check, so a worker that did not itself run the migrations (or an + operator's out-of-band ``make migrate``) still finishes starting its + modules. Serialised per app; a hook that fails again is logged and dropped + rather than raised, because this runs after the migrations have already + committed and a 500 there would help nobody. + """ + deferred = getattr(app.state, "deferred_startup", None) + if not deferred: + return + lock = getattr(app.state, "deferred_startup_lock", None) + if lock is None: # no await between check and set, so this cannot race + lock = app.state.deferred_startup_lock = asyncio.Lock() + async with lock: + if not deferred: + return + await hydrate_settings_from_db(app) + while deferred: + mod = deferred.pop(0) + try: + await mod.on_startup(app) + except Exception: + logger.exception("Deferred on_startup of %s failed after migrations", mod.meta.name) diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index 8a19b4db3..4db296b79 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -3,228 +3,24 @@ from __future__ import annotations import logging -from collections.abc import Mapping -from http import HTTPStatus from fastapi.encoders import jsonable_encoder from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse from simple_module_core.exceptions import NotFoundError from simple_module_core.permission_denied import PermissionDenied -from simple_module_inertia import ( - Inertia, - InertiaConfig, - InertiaVersionConflictException, - inertia_version_conflict_exception_handler, -) from starlette.exceptions import HTTPException from starlette.requests import Request from starlette.responses import Response -from simple_module_hosting._inertia_shared import _INERTIA_HEADER +from simple_module_hosting._error_negotiation import _wants_json +from simple_module_hosting._error_page import render_error_page from simple_module_hosting._observability import CorrelationIdMiddleware logger = logging.getLogger(__name__) _INERTIA_ERROR_STATUSES = frozenset({401, 403, 404, 413, 419, 422, 429, 500, 503}) -# Statuses whose remedy is "sign in", so the page offers that as its primary -# action rather than sending the visitor to the landing page. -_SIGN_IN_STATUSES = frozenset({401, 419}) - - -def _specific_detail(status_code: int, message: str) -> str: - """The detail, unless it only restates the status. - - Starlette defaults ``HTTPException.detail`` to ``HTTPStatus(code).phrase``, - so a plain ``HTTPException(404)`` reaches the page carrying "Not Found" and - a bare ``HTTPException(403)`` carrying "Forbidden". The page prefers a - server message over its own catalog description — right when the message - says something ("Administrator access required"), useless when it is the - title again in Title Case, and it made the page's own 404/403 sentences - unreachable on the very paths that produce them. - - Only an exact, case-insensitive match is dropped: anything a caller - actually wrote survives, including a sentence that merely starts with the - phrase. Non-standard codes (419) have no phrase to collide with. - """ - try: - phrase = HTTPStatus(status_code).phrase - except ValueError: - return message - return "" if message.strip().casefold() == phrase.casefold() else message - - -def _login_url(request: Request) -> str | None: - """Best-effort login URL for the sign-in statuses. - - Read off ``app.state`` rather than imported: the auth provider is a plugin - concern and ``SM009`` forbids framework code importing ``modules/*``. An - app with no auth provider installed simply gets no sign-in button. - """ - auth_state = getattr(request.app.state, "auth", None) - provider = getattr(auth_state, "auth_provider", None) - if provider is None: - return None - try: - return provider.get_login_url(request) - except Exception: - # A broken provider must not turn an error page into a second error. - logger.exception("Auth provider failed to supply a login URL") - return None - - -def _explicit_accept_q(accept: str, media_type: str) -> float | None: - """Quality the Accept header gives ``media_type`` by explicit listing, or - ``None`` when the caller never named it at all. - - Only exact entries count (no ``*/*`` / ``type/*`` wildcards) — the - negotiation below cares whether the caller *named* html or json, and at - what preference, and separately whether they named it *at all* (``None`` - vs. an explicit ``q=0``). Malformed q-values fall back to 1.0 per RFC 9110. - """ - for part in accept.split(","): - media, _, params = part.partition(";") - if media.strip().lower() != media_type: - continue - q = 1.0 - for param in params.split(";"): - name, _, value = param.partition("=") - if name.strip().lower() == "q": - try: - q = float(value.strip()) - except ValueError: - q = 1.0 - return q - return None - - -def _wants_json(request: Request) -> bool: - """API callers get JSON error bodies; browser-shaped requests get the page. - - A request that explicitly accepts ``text/html`` is a browser navigation — - and those reach ``/api/*`` too (OAuth login links, file-download hrefs) — - so it always gets the rendered page, unless the caller *prefers* JSON via - q-values (``application/json, text/html;q=0.5`` is an API client keeping - an html fallback). So does an Inertia visit (``X-Inertia`` header), - whatever its Accept — an Inertia client can only consume Inertia-protocol - responses, never a bare JSON body. Otherwise ``/api/*``, the documented - prefix for every module's JSON surface (``ModuleMeta.route_prefix``), - gets JSON — a bare ``fetch()`` sends ``Accept: */*`` — as does an - explicit ``Accept: application/json`` anywhere else. - - ``text/html;q=0`` rules html out *everywhere*, not just under ``/api/*``: - once the caller has explicitly said they won't accept html, this never - chooses the html page. The (already-moot) html fallback survives only - when json wasn't mentioned either — there's nothing else to offer, so a - view path keeps its pre-existing default and an ``/api/*`` path keeps - getting JSON either way. - """ - if request.headers.get(_INERTIA_HEADER): - return False - accept = request.headers.get("accept", "") - html_q = _explicit_accept_q(accept, "text/html") - json_q = _explicit_accept_q(accept, "application/json") - if html_q is not None and html_q > 0 and html_q >= (json_q or 0.0): - return False - path = request.url.path - is_api = path == "/api" or path.startswith("/api/") - if html_q == 0: - return is_api if json_q is None else True - return is_api or bool(json_q) - - -async def render_error_page( - request: Request, - status_code: int, - message: str, - headers: Mapping[str, str] | None = None, - required_permission: str | None = None, -) -> Response: - """Render the Inertia error page for *status_code*. - - ``required_permission`` names the permission a 403 lacked (from - :class:`PermissionDenied`); the page words its own sentence around it. - - ``headers`` carries an ``HTTPException``'s own headers through to the - response. A rendered page is still the same status as the JSON body it - replaces, so ``WWW-Authenticate`` on a 401 and ``Retry-After`` on a - 429/503 have to survive the switch — widening the set of statuses that - render a page must not quietly narrow what those responses carry. - """ - try: - # Inside the try, not above it: this lookup is exactly the kind of - # thing that is missing when the app is half-built, and an error page - # that raises while reporting an error leaves the caller with nothing. - config: InertiaConfig = request.app.state.sm.inertia_config - # Prefer the app's configured dependency over constructing Inertia - # directly, so the error page is built the same way as every other - # page. The raw construction is the fallback for a half-built app — - # the case this whole handler exists to survive. Both paths emit a - # root-relative page url: the adapter does that by construction now, - # so the fallback no longer needs a separate patch to avoid the - # cross-scheme `pushState` SecurityError behind a TLS proxy. - inertia_dep = getattr(request.app.state, "inertia_dependency", None) - if inertia_dep is not None: - inertia = inertia_dep(request, None) - else: - inertia = Inertia(request, config) - # This does not go through get_inertia, so the share step has to be - # repeated here. Without it the error page renders raw translation keys - # (ui.error_page.not_found_title) and loses auth/menus, so a signed-in - # user's 404 has no layout. - shared = getattr(request.state, "inertia_shared", None) - if shared: - inertia.share(**shared) - # The correlation id is the only handle a user has on their own failed - # request — without it a support report is just "it broke". It is already - # on every log line for this request, so quoting it back makes the page - # and the logs joinable. - response = await inertia.render( - "Error", - { - "status": status_code, - # Not the raw detail: a default one is just the status phrase, - # and the page renders any message it is given ahead of its own - # copy. See `_specific_detail`. - "message": _specific_detail(status_code, message), - "correlation_id": getattr(request.state, "correlation_id", "") or "", - # Always present, null when no permission is involved: the page - # branches on it, and an absent prop is indistinguishable there - # from one that was never wired. - "required_permission": required_permission, - "login_url": (_login_url(request) if status_code in _SIGN_IN_STATUSES else None), - # Set by MaintenanceMiddleware. A planned outage reads very - # differently from the same status code arriving unbidden. - "maintenance": bool(getattr(request.state, "maintenance", False)), - }, - ) - response.status_code = status_code - _apply_headers(response, headers) - return response - except InertiaVersionConflictException as exc: - return await inertia_version_conflict_exception_handler(request, exc) - except Exception: - # Fallback if Inertia rendering itself fails (e.g. missing session) - logger.exception("Error page rendering failed, falling back to JSON") - return JSONResponse( - status_code=status_code, - content={"detail": message or "Internal Server Error"}, - headers=dict(headers) if headers else None, - ) - - -def _apply_headers(response: Response, headers: Mapping[str, str] | None) -> None: - """Copy exception headers onto an already-built response. - - Set rather than appended: these are single-valued response headers, and - a duplicate ``Retry-After`` is worse than none. - """ - if not headers: - return - for key, value in headers.items(): - response.headers[key] = value - async def http_exception_handler(request: Request, exc: HTTPException) -> Response: # Preserve exception headers (WWW-Authenticate, Retry-After, ...) the way diff --git a/framework/hosting/simple_module_hosting/_error_negotiation.py b/framework/hosting/simple_module_hosting/_error_negotiation.py new file mode 100644 index 000000000..7ecee488e --- /dev/null +++ b/framework/hosting/simple_module_hosting/_error_negotiation.py @@ -0,0 +1,65 @@ +"""Choose JSON or HTML for framework error responses.""" + +from starlette.requests import Request + +from simple_module_hosting._inertia_shared import _INERTIA_HEADER + + +def _explicit_accept_q(accept: str, media_type: str) -> float | None: + """Quality the Accept header gives ``media_type`` by explicit listing, or + ``None`` when the caller never named it at all. + + Only exact entries count (no ``*/*`` / ``type/*`` wildcards) — the + negotiation below cares whether the caller *named* html or json, and at + what preference, and separately whether they named it *at all* (``None`` + vs. an explicit ``q=0``). Malformed q-values fall back to 1.0 per RFC 9110. + """ + for part in accept.split(","): + media, _, params = part.partition(";") + if media.strip().lower() != media_type: + continue + q = 1.0 + for param in params.split(";"): + name, _, value = param.partition("=") + if name.strip().lower() == "q": + try: + q = float(value.strip()) + except ValueError: + q = 1.0 + return q + return None + + +def _wants_json(request: Request) -> bool: + """API callers get JSON error bodies; browser-shaped requests get the page. + + A request that explicitly accepts ``text/html`` is a browser navigation — + and those reach ``/api/*`` too (OAuth login links, file-download hrefs) — + so it always gets the rendered page, unless the caller *prefers* JSON via + q-values (``application/json, text/html;q=0.5`` is an API client keeping + an html fallback). So does an Inertia visit (``X-Inertia`` header), + whatever its Accept — an Inertia client can only consume Inertia-protocol + responses, never a bare JSON body. Otherwise ``/api/*``, the documented + prefix for every module's JSON surface (``ModuleMeta.route_prefix``), + gets JSON — a bare ``fetch()`` sends ``Accept: */*`` — as does an + explicit ``Accept: application/json`` anywhere else. + + ``text/html;q=0`` rules html out *everywhere*, not just under ``/api/*``: + once the caller has explicitly said they won't accept html, this never + chooses the html page. The (already-moot) html fallback survives only + when json wasn't mentioned either — there's nothing else to offer, so a + view path keeps its pre-existing default and an ``/api/*`` path keeps + getting JSON either way. + """ + if request.headers.get(_INERTIA_HEADER): + return False + accept = request.headers.get("accept", "") + html_q = _explicit_accept_q(accept, "text/html") + json_q = _explicit_accept_q(accept, "application/json") + if html_q is not None and html_q > 0 and html_q >= (json_q or 0.0): + return False + path = request.url.path + is_api = path == "/api" or path.startswith("/api/") + if html_q == 0: + return is_api if json_q is None else True + return is_api or bool(json_q) diff --git a/framework/hosting/simple_module_hosting/_error_page.py b/framework/hosting/simple_module_hosting/_error_page.py new file mode 100644 index 000000000..7ffffe102 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_error_page.py @@ -0,0 +1,156 @@ +"""Render error responses without depending on handlers or middleware wiring.""" + +from __future__ import annotations + +import logging +from collections.abc import Mapping +from http import HTTPStatus + +from fastapi.responses import JSONResponse +from simple_module_inertia import ( + Inertia, + InertiaConfig, + InertiaVersionConflictException, + inertia_version_conflict_exception_handler, +) +from starlette.requests import Request +from starlette.responses import Response + +logger = logging.getLogger(__name__) + +# Statuses whose remedy is "sign in", so the page offers that as its primary +# action rather than sending the visitor to the landing page. +_SIGN_IN_STATUSES = frozenset({401, 419}) + + +def _specific_detail(status_code: int, message: str) -> str: + """The detail, unless it only restates the status. + + Starlette defaults ``HTTPException.detail`` to ``HTTPStatus(code).phrase``, + so a plain ``HTTPException(404)`` reaches the page carrying "Not Found" and + a bare ``HTTPException(403)`` carrying "Forbidden". The page prefers a + server message over its own catalog description — right when the message + says something ("Administrator access required"), useless when it is the + title again in Title Case, and it made the page's own 404/403 sentences + unreachable on the very paths that produce them. + + Only an exact, case-insensitive match is dropped: anything a caller + actually wrote survives, including a sentence that merely starts with the + phrase. Non-standard codes (419) have no phrase to collide with. + """ + try: + phrase = HTTPStatus(status_code).phrase + except ValueError: + return message + return "" if message.strip().casefold() == phrase.casefold() else message + + +def _login_url(request: Request) -> str | None: + """Best-effort login URL for the sign-in statuses. + + Read off ``app.state`` rather than imported: the auth provider is a plugin + concern and ``SM009`` forbids framework code importing ``modules/*``. An + app with no auth provider installed simply gets no sign-in button. + """ + auth_state = getattr(request.app.state, "auth", None) + provider = getattr(auth_state, "auth_provider", None) + if provider is None: + return None + try: + return provider.get_login_url(request) + except Exception: + # A broken provider must not turn an error page into a second error. + logger.exception("Auth provider failed to supply a login URL") + return None + + +async def render_error_page( + request: Request, + status_code: int, + message: str, + headers: Mapping[str, str] | None = None, + required_permission: str | None = None, +) -> Response: + """Render the Inertia error page for *status_code*. + + ``required_permission`` names the permission a 403 lacked (from + :class:`PermissionDenied`); the page words its own sentence around it. + + ``headers`` carries an ``HTTPException``'s own headers through to the + response. A rendered page is still the same status as the JSON body it + replaces, so ``WWW-Authenticate`` on a 401 and ``Retry-After`` on a + 429/503 have to survive the switch — widening the set of statuses that + render a page must not quietly narrow what those responses carry. + """ + try: + # Inside the try, not above it: this lookup is exactly the kind of + # thing that is missing when the app is half-built, and an error page + # that raises while reporting an error leaves the caller with nothing. + config: InertiaConfig = request.app.state.sm.inertia_config + # Prefer the app's configured dependency over constructing Inertia + # directly, so the error page is built the same way as every other + # page. The raw construction is the fallback for a half-built app — + # the case this whole handler exists to survive. Both paths emit a + # root-relative page url: the adapter does that by construction now, + # so the fallback no longer needs a separate patch to avoid the + # cross-scheme `pushState` SecurityError behind a TLS proxy. + inertia_dep = getattr(request.app.state, "inertia_dependency", None) + if inertia_dep is not None: + inertia = inertia_dep(request, None) + else: + inertia = Inertia(request, config) + # This does not go through get_inertia, so the share step has to be + # repeated here. Without it the error page renders raw translation keys + # (ui.error_page.not_found_title) and loses auth/menus, so a signed-in + # user's 404 has no layout. + shared = getattr(request.state, "inertia_shared", None) + if shared: + inertia.share(**shared) + # The correlation id is the only handle a user has on their own failed + # request — without it a support report is just "it broke". It is already + # on every log line for this request, so quoting it back makes the page + # and the logs joinable. + response = await inertia.render( + "Error", + { + "status": status_code, + # Not the raw detail: a default one is just the status phrase, + # and the page renders any message it is given ahead of its own + # copy. See `_specific_detail`. + "message": _specific_detail(status_code, message), + "correlation_id": getattr(request.state, "correlation_id", "") or "", + # Always present, null when no permission is involved: the page + # branches on it, and an absent prop is indistinguishable there + # from one that was never wired. + "required_permission": required_permission, + "login_url": (_login_url(request) if status_code in _SIGN_IN_STATUSES else None), + # Set by MaintenanceMiddleware. A planned outage reads very + # differently from the same status code arriving unbidden. + "maintenance": bool(getattr(request.state, "maintenance", False)), + }, + ) + response.status_code = status_code + _apply_headers(response, headers) + return response + except InertiaVersionConflictException as exc: + return await inertia_version_conflict_exception_handler(request, exc) + except Exception: + # Fallback if Inertia rendering itself fails (e.g. missing session) + logger.exception("Error page rendering failed, falling back to JSON") + return JSONResponse( + status_code=status_code, + content={"detail": message or "Internal Server Error"}, + headers=dict(headers) if headers else None, + ) + + +def _apply_headers(response: Response, headers: Mapping[str, str] | None) -> None: + """Copy exception headers onto an already-built response. + + Set rather than appended: these are single-valued response headers, and + a duplicate ``Retry-After`` is worse than none. + """ + if not headers: + return + for key, value in headers.items(): + response.headers[key] = value diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index 934e01498..b3623dda6 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -13,16 +13,13 @@ from __future__ import annotations -import asyncio import logging from collections.abc import AsyncGenerator, Callable, Sequence from contextlib import asynccontextmanager from fastapi import FastAPI -from simple_module_core.module_settings import get_module_settings_registry -from simple_module_db.stored_settings import SessionOverrideSource, settings_table_exists -from simple_module_hosting._hydrate_step import hydrate_all +from simple_module_hosting._deferred_startup import hydrate_settings_from_db from simple_module_hosting._route_guard import guard_included_routers from simple_module_hosting.migrations import migration_status from simple_module_hosting.setup_gate import STEP_MIGRATIONS @@ -30,28 +27,6 @@ logger = logging.getLogger(__name__) -async def hydrate_settings_from_db(app: FastAPI) -> None: - """Merge DB-stored overrides into every registered settings object. - - Reads the settings table directly (``simple_module_db.stored_settings``), - so it works whether or not the ``settings`` plugin is enabled (#500). A - missing table is a fresh install: nothing is stored yet, so there is - nothing to merge. - - Note this is the *second* settings read of a boot. The first happens in - ``_preapp_config`` before ``create_app`` builds anything, because the - module list, i18n registry and middleware stack are all constructed from - settings and cannot be rebuilt from here. - """ - if get_module_settings_registry(app) is None: - return - - async with app.state.sm.db.session_factory() as session: - if not await settings_table_exists(session): - return - await hydrate_all(app, SessionOverrideSource(session), session) - - async def _is_first_run(app: FastAPI) -> bool: """Whether this install has never been set up at all. @@ -116,34 +91,6 @@ async def _is_first_run(app: FastAPI) -> bool: return False -async def run_deferred_startup(app: FastAPI) -> None: - """Replay the ``on_startup`` hooks that could not run on an unmigrated DB. - - Called from the wizard's migrations action and from the setup gate's - schema re-check, so a worker that did not itself run the migrations (or an - operator's out-of-band ``make migrate``) still finishes starting its - modules. Serialised per app; a hook that fails again is logged and dropped - rather than raised, because this runs after the migrations have already - committed and a 500 there would help nobody. - """ - deferred = getattr(app.state, "deferred_startup", None) - if not deferred: - return - lock = getattr(app.state, "deferred_startup_lock", None) - if lock is None: # no await between check and set, so this cannot race - lock = app.state.deferred_startup_lock = asyncio.Lock() - async with lock: - if not deferred: - return - await hydrate_settings_from_db(app) - while deferred: - mod = deferred.pop(0) - try: - await mod.on_startup(app) - except Exception: - logger.exception("Deferred on_startup of %s failed after migrations", mod.meta.name) - - def build_lifespan(modules: Sequence) -> Callable: """Return the ``lifespan`` context manager for an app over *modules*.""" diff --git a/framework/hosting/simple_module_hosting/_request_guard_responses.py b/framework/hosting/simple_module_hosting/_request_guard_responses.py index d6ea641b3..0f970e583 100644 --- a/framework/hosting/simple_module_hosting/_request_guard_responses.py +++ b/framework/hosting/simple_module_hosting/_request_guard_responses.py @@ -15,7 +15,8 @@ from starlette.responses import JSONResponse, Response from starlette.types import Receive, Scope, Send -from simple_module_hosting._error_handlers import _wants_json, render_error_page +from simple_module_hosting._error_negotiation import _wants_json +from simple_module_hosting._error_page import render_error_page async def guard_response( diff --git a/framework/hosting/simple_module_hosting/maintenance.py b/framework/hosting/simple_module_hosting/maintenance.py index 8de2ad134..4b99ecb3b 100644 --- a/framework/hosting/simple_module_hosting/maintenance.py +++ b/framework/hosting/simple_module_hosting/maintenance.py @@ -17,8 +17,12 @@ from starlette.responses import JSONResponse from starlette.types import ASGIApp, Receive, Scope, Send +from simple_module_hosting._error_negotiation import _wants_json +from simple_module_hosting._error_page import render_error_page from simple_module_hosting.public_access import is_public +logger = logging.getLogger(__name__) + # Kept reachable while the gate is closed: liveness probes (so orchestrators # do not kill the pod mid-maintenance), static assets and i18n bundles (or the # 503 page renders unstyled and untranslated). @@ -90,11 +94,6 @@ def _may_bypass(request: Request, scope: Scope) -> bool: @staticmethod async def _render(request: Request, message: str): - # Imported here rather than at module scope: _error_handlers imports - # from inertia, and a circular import at boot is a worse failure than - # a per-request attribute lookup. - from simple_module_hosting._error_handlers import _wants_json, render_error_page - if _wants_json(request): return JSONResponse( status_code=503, diff --git a/framework/hosting/simple_module_hosting/sentry.py b/framework/hosting/simple_module_hosting/sentry.py index a8796d299..035b9291a 100644 --- a/framework/hosting/simple_module_hosting/sentry.py +++ b/framework/hosting/simple_module_hosting/sentry.py @@ -46,6 +46,19 @@ _enabled = False +def reset_sentry() -> None: + """Disable reporting and discard request context between isolated app runs.""" + global _enabled + if _enabled: + import sentry_sdk + + client = sentry_sdk.get_global_scope().client + client.close() + sentry_sdk.get_global_scope().set_client(None) + _enabled = False + _request_scope.set(None) + + def is_enabled() -> bool: return _enabled diff --git a/framework/hosting/simple_module_hosting/setup_gate.py b/framework/hosting/simple_module_hosting/setup_gate.py index 676ff24e7..2aa7dc5e4 100644 --- a/framework/hosting/simple_module_hosting/setup_gate.py +++ b/framework/hosting/simple_module_hosting/setup_gate.py @@ -20,7 +20,11 @@ from starlette.responses import RedirectResponse from starlette.types import ASGIApp, Receive, Scope, Send +from simple_module_hosting import migrations +from simple_module_hosting._deferred_startup import run_deferred_startup + logger = logging.getLogger(__name__) +_migration_failure_warned = False SETUP_PATH = "/setup" @@ -71,20 +75,21 @@ async def _database_migrated(app) -> bool: if status.get("is_current", True): return True - from simple_module_hosting.migrations import migration_status - try: - status = await migration_status(app.state.sm.db.engine) - except Exception as exc: - logger.debug("Migration re-check failed, using the boot snapshot: %s", exc) + status = await migrations.migration_status(app.state.sm.db.engine) + except Exception: + global _migration_failure_warned + if not _migration_failure_warned: + _migration_failure_warned = True + logger.warning( + "Migration re-check failed; retaining the incomplete boot snapshot", exc_info=True + ) return False app.state.migration = status if status["is_current"]: # Finish starting the modules whose on_startup needed these tables — # this worker may not be the one that ran the migrations. Never lets # a failure there change the verdict, which only reports the schema. - from simple_module_hosting._lifespan import run_deferred_startup - try: await run_deferred_startup(app) except Exception: diff --git a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py index 3b214b39b..0fc41fa67 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -15,6 +15,8 @@ from fastapi import HTTPException, Request +from simple_module_hosting._deferred_startup import run_deferred_startup + logger = logging.getLogger(__name__) # The endpoint is anonymous, so nothing stops two requests (a double click, a @@ -85,8 +87,6 @@ def _upgrade() -> None: request.app.state.migration = await migration_status(request.app.state.sm.db.engine) # Modules whose on_startup could not run before the tables existed. - from simple_module_hosting._lifespan import run_deferred_startup - await run_deferred_startup(request.app) logger.info("Setup: migrations applied") return {"migration": request.app.state.migration} diff --git a/framework/hosting/tests/test_error_status_coverage.py b/framework/hosting/tests/test_error_status_coverage.py index a4cd00f43..8590504bb 100644 --- a/framework/hosting/tests/test_error_status_coverage.py +++ b/framework/hosting/tests/test_error_status_coverage.py @@ -16,9 +16,8 @@ import pytest from simple_module_hosting._error_handlers import ( _INERTIA_ERROR_STATUSES, - _SIGN_IN_STATUSES, - _login_url, ) +from simple_module_hosting._error_page import _SIGN_IN_STATUSES, _login_url _ROOT = Path(__file__).resolve().parents[3] # The page body ships in @simple-module-py/ui (host/pages/Error.tsx just re-exports it). diff --git a/framework/hosting/tests/test_hosting_error_handling_policy.py b/framework/hosting/tests/test_hosting_error_handling_policy.py new file mode 100644 index 000000000..50fded96c --- /dev/null +++ b/framework/hosting/tests/test_hosting_error_handling_policy.py @@ -0,0 +1,72 @@ +"""Fallbacks preserve their outcomes while warning once with tracebacks.""" + +import logging +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest +from simple_module_core import _i18n_translator, invalidation, menu +from simple_module_db import listeners +from simple_module_hosting import setup_gate + + +def _assert_warning(caplog, message): + records = [record for record in caplog.records if message in record.getMessage()] + assert len(records) == 1 + assert records[0].levelno == logging.WARNING + assert records[0].exc_info is not None + + +def test_plural_failure_warns_once_and_uses_other(monkeypatch, caplog): + monkeypatch.setattr(_i18n_translator, "_plural_failure_warned", False) + for _ in range(2): + assert _i18n_translator._plural_form("not-a-real-locale", 1) == "other" + _assert_warning(caplog, "Plural locale parsing failed") + + +def test_audit_identity_failure_warns_once_and_omits_key(monkeypatch, caplog): + monkeypatch.setattr(listeners, "_entity_pk_failure_warned", False) + for _ in range(2): + assert listeners._entity_pk(object()) is None + _assert_warning(caplog, "Audit identity inspection failed") + + +def test_invalidation_decoder_warns_once_and_drops_payload(monkeypatch, caplog): + monkeypatch.setattr(invalidation, "_decode_failure_warned", False) + for _ in range(2): + assert invalidation.Invalidation.from_wire("invalid json") is None + _assert_warning(caplog, "Ignoring undecodable invalidation message") + assert invalidation.Invalidation.from_wire(invalidation.Invalidation("cache").to_wire()) + + +@pytest.mark.asyncio +async def test_menu_failure_warns_once_per_provider_and_keeps_other_items(caplog): + registry = menu.MenuRegistry() + + def broken(request): + yield menu.MenuItem(label="Partial", url="/partial") + raise RuntimeError("broken provider") + + item = menu.MenuItem(label="Healthy", url="/healthy") + registry.add_provider(broken) + registry.add_provider(lambda request: [item]) + for _ in range(2): + assert await registry.collect_provider_items(SimpleNamespace()) == [item] + _assert_warning(caplog, "Menu provider") + + +@pytest.mark.asyncio +async def test_migration_failure_warns_once_and_keeps_gate_closed(monkeypatch, caplog): + monkeypatch.setattr(setup_gate, "_migration_failure_warned", False) + check = AsyncMock(side_effect=RuntimeError("database unavailable")) + monkeypatch.setattr(setup_gate, "migration_status", check) + snapshot = {"is_current": False} + app = SimpleNamespace( + state=SimpleNamespace( + migration=snapshot, sm=SimpleNamespace(db=SimpleNamespace(engine=None)) + ) + ) + for _ in range(2): + assert not await setup_gate._database_migrated(app) + assert app.state.migration is snapshot + _assert_warning(caplog, "Migration re-check failed") diff --git a/framework/hosting/tests/test_lifespan_deferred_startup.py b/framework/hosting/tests/test_lifespan_deferred_startup.py index 898a066f0..1e03d7f9d 100644 --- a/framework/hosting/tests/test_lifespan_deferred_startup.py +++ b/framework/hosting/tests/test_lifespan_deferred_startup.py @@ -8,7 +8,8 @@ import pytest from fastapi import FastAPI -from simple_module_hosting._lifespan import build_lifespan, run_deferred_startup +from simple_module_hosting._deferred_startup import run_deferred_startup +from simple_module_hosting._lifespan import build_lifespan def _module(name: str, *, fail_first: bool) -> SimpleNamespace: @@ -52,7 +53,7 @@ async def test_failing_hook_is_deferred_then_replayed_on_unmigrated_first_run(): async def capture(app_): seen["deferred"] = [m.meta.name for m in app_.state.deferred_startup] - with patch("simple_module_hosting._lifespan.hydrate_settings_from_db", AsyncMock()): + with patch("simple_module_hosting._deferred_startup.hydrate_settings_from_db", AsyncMock()): status = {"is_current": False, "pending_count": 1} with ( patch( @@ -86,7 +87,7 @@ async def boom(app_): bad.on_startup = boom ok = _module("ok", fail_first=False) app.state.deferred_startup = [bad, ok] - with patch("simple_module_hosting._lifespan.hydrate_settings_from_db", AsyncMock()): + with patch("simple_module_hosting._deferred_startup.hydrate_settings_from_db", AsyncMock()): await asyncio.gather(run_deferred_startup(app), run_deferred_startup(app)) assert bad.calls["n"] == 1 diff --git a/framework/hosting/tests/test_menu_providers_wiring.py b/framework/hosting/tests/test_menu_providers_wiring.py index e8c7082ce..d3f65cbdc 100644 --- a/framework/hosting/tests/test_menu_providers_wiring.py +++ b/framework/hosting/tests/test_menu_providers_wiring.py @@ -96,7 +96,7 @@ async def boom(_r: Request) -> list[MenuItem]: menu.add_provider(boom) menu.add_provider(lambda _r: [MenuItem(label="Ok", url="/ok", order=-1)]) - with caplog.at_level(logging.ERROR): + with caplog.at_level(logging.WARNING): resp = _build(menu).get("/shared", headers={"x-role": "user"}) assert resp.status_code == 200 assert _labels(resp.json()) == ["Ok", "Static"] diff --git a/framework/hosting/tests/test_route_guard_fastapi_contract.py b/framework/hosting/tests/test_route_guard_fastapi_contract.py index f5b686206..0af0cde18 100644 --- a/framework/hosting/tests/test_route_guard_fastapi_contract.py +++ b/framework/hosting/tests/test_route_guard_fastapi_contract.py @@ -126,3 +126,34 @@ async def item(): included_type(original_router=leaf, include_context=child) # constructible by keyword build = getattr(included, "_build_effective_context", None) _accepts(build, "_IncludedRouter._build_effective_context(route)", route) + + +def test_starlette_route_matches_contract(): + from starlette.routing import Route + + async def endpoint(request): + return None + + route = Route("/api/items/{item_id:int}", endpoint, methods=["GET"]) + scope = {"type": "http", "path": "/api/items/42", "root_path": "", "method": "GET"} + parameters = tuple(inspect.signature(Route.matches).parameters.values()) + assert tuple(p.name for p in parameters) == ("self", "scope"), ( + f"Route.matches must retain its (self, scope) signature; update {GUARD}" + ) + matches = _accepts(route.matches, "Route.matches(scope)", scope) + match, child_scope = matches(scope) + assert match is Match.FULL + assert child_scope["endpoint"] is endpoint + assert child_scope["path_params"] == {"item_id": 42} + partial, partial_scope = matches({**scope, "method": "POST"}) + assert partial is Match.PARTIAL + assert partial_scope == child_scope + assert matches({**scope, "path": "/elsewhere"}) == (Match.NONE, {}) + assert matches({**scope, "type": "websocket"}) == (Match.NONE, {}) + + def replacement(scope): + return Match.NONE, {} + + route.matches = replacement + assert route.matches is replacement + assert route.matches(scope) == (Match.NONE, {}) diff --git a/framework/testing/simple_module_test/fixtures.py b/framework/testing/simple_module_test/fixtures.py index 53989ab1f..53029b13c 100644 --- a/framework/testing/simple_module_test/fixtures.py +++ b/framework/testing/simple_module_test/fixtures.py @@ -161,6 +161,9 @@ async def _seed_setup_admin(application) -> None: async def _build_app(settings: Settings, *, seed_admin: bool): from simple_module_hosting.app_builder import create_app + from simple_module_test.process_state import reset_process_state + + reset_process_state() application = create_app(settings) await _create_all_tables(application.state.sm.db.engine) diff --git a/framework/testing/simple_module_test/mail.py b/framework/testing/simple_module_test/mail.py index 076905233..05bfbf8c1 100644 --- a/framework/testing/simple_module_test/mail.py +++ b/framework/testing/simple_module_test/mail.py @@ -10,8 +10,7 @@ from collections.abc import Iterator import pytest -from simple_module_core import mail as core_mail -from simple_module_core.mail import MailMessage +from simple_module_core.mail import MailMessage, install_mail_sender class CapturedMail: @@ -28,14 +27,6 @@ def to(self, address: str) -> list[MailMessage]: return [m for m in self.messages if m.to == address] -@pytest.fixture(autouse=True) -def _reset_process_mail_sender() -> Iterator[None]: - sender, resolver = core_mail._sender, core_mail._resolver - yield - core_mail.install_mail_sender(sender) - core_mail.install_mail_resolver(resolver) - - @pytest.fixture def captured_mail() -> Iterator[CapturedMail]: """Install a recording sender that claims to deliver; yields it. @@ -45,5 +36,5 @@ def captured_mail() -> Iterator[CapturedMail]: reinstalls the real one; reinstall with ``install_mail_sender(captured)``. """ captured = CapturedMail() - core_mail.install_mail_sender(captured) + install_mail_sender(captured) yield captured diff --git a/framework/testing/simple_module_test/plugin.py b/framework/testing/simple_module_test/plugin.py index 2e8f52b0b..1cb870961 100644 --- a/framework/testing/simple_module_test/plugin.py +++ b/framework/testing/simple_module_test/plugin.py @@ -33,7 +33,8 @@ settings, setup_pending_app, ) -from simple_module_test.mail import _reset_process_mail_sender, captured_mail # noqa: F401 +from simple_module_test.mail import captured_mail # noqa: F401 +from simple_module_test.process_state import _reset_process_state # noqa: F401 from simple_module_test.redis_server import redis_server # noqa: F401 from simple_module_test.tenant_client import tenant_client # noqa: F401 diff --git a/framework/testing/simple_module_test/process_state.py b/framework/testing/simple_module_test/process_state.py new file mode 100644 index 000000000..63f3ab2b6 --- /dev/null +++ b/framework/testing/simple_module_test/process_state.py @@ -0,0 +1,24 @@ +"""Reset process services between tests and apps built by the test plugin.""" + +from collections.abc import Iterator + +import pytest +from simple_module_core.leases import reset_lease_backend +from simple_module_core.mail import reset_mail_sender +from simple_module_hosting.sentry import reset_sentry + + +def reset_process_state() -> None: + """Restore unconfigured services without retaining another app's resources.""" + reset_mail_sender() + reset_lease_backend() + reset_sentry() + + +@pytest.fixture(autouse=True) +def _reset_process_state() -> Iterator[None]: + reset_process_state() + try: + yield + finally: + reset_process_state() diff --git a/framework/testing/tests/test_testing_process_state.py b/framework/testing/tests/test_testing_process_state.py new file mode 100644 index 000000000..4ea442716 --- /dev/null +++ b/framework/testing/tests/test_testing_process_state.py @@ -0,0 +1,93 @@ +"""Process services are isolated between plugin-built apps and tests.""" + +from unittest.mock import Mock + +from simple_module_core.leases import ( + InProcessLeaseBackend, + configure_leases, + get_leases, + reset_lease_backend, +) +from simple_module_core.mail import ( + ConsoleMailSender, + current_mail_sender, + install_mail_resolver, + install_mail_sender, + mail_transport_installed, + reset_mail_sender, + resolve_mail_sender, +) +from simple_module_hosting import sentry +from simple_module_test.mail import CapturedMail +from simple_module_test.process_state import reset_process_state + + +async def test_reset_mail_clears_sender_and_worker_resolver() -> None: + sender = CapturedMail() + + async def resolver(db): + return sender + + install_mail_sender(sender) + install_mail_resolver(resolver) + reset_mail_sender() + assert not mail_transport_installed() + assert isinstance(current_mail_sender(), ConsoleMailSender) + assert isinstance(await resolve_mail_sender(object()), ConsoleMailSender) + assert current_mail_sender().include_body is False + + +async def test_reset_leases_discards_configuration_and_held_names() -> None: + original = InProcessLeaseBackend() + configure_leases(original) + assert await original.acquire("held", "previous-app", 60) == 1 + reset_lease_backend() + replacement = get_leases() + assert replacement is not original + assert isinstance(replacement, InProcessLeaseBackend) + assert await replacement.acquire("held", "next-app", 60) == 1 + + +def test_reset_sentry_closes_and_unbinds_client(monkeypatch) -> None: + import pytest + + sdk = pytest.importorskip("sentry_sdk") + scope = Mock() + monkeypatch.setattr(sdk, "get_global_scope", lambda: scope) + monkeypatch.setattr(sentry, "_enabled", True) + sentry._request_scope.set({"type": "http"}) + sentry.reset_sentry() + scope.client.close.assert_called_once_with() + scope.set_client.assert_called_once_with(None) + assert not sentry.is_enabled() + assert sentry._request_scope.get() is None + sentry.reset_sentry() + scope.client.close.assert_called_once_with() + + +async def test_plugin_app_builder_resets_services_between_apps(settings, monkeypatch) -> None: + from fastapi import FastAPI + from simple_module_hosting import app_builder + from simple_module_test import fixtures + + seen = [] + + def create_app(settings): + seen.append((mail_transport_installed(), get_leases(), sentry.is_enabled())) + app = FastAPI() + app.state.sm = Mock() + install_mail_sender(CapturedMail()) + return app + + async def create_tables(engine): + pass + + monkeypatch.setattr(app_builder, "create_app", create_app) + monkeypatch.setattr(fixtures, "_create_all_tables", create_tables) + for _ in range(2): + async for _app in fixtures._build_app(settings, seed_admin=False): + pass + assert len(seen) == 2 + assert all(not mail and not enabled for mail, _, enabled in seen) + assert seen[0][1] is not seen[1][1] + reset_process_state() diff --git a/host/tests/test_deprecation_expiry.py b/host/tests/test_deprecation_expiry.py index 3d4712fac..202e80718 100644 --- a/host/tests/test_deprecation_expiry.py +++ b/host/tests/test_deprecation_expiry.py @@ -27,6 +27,7 @@ "settings.hydrate", "simple_module_hosting._registrations", "simple_module_core.public_routes", + "simple_module_core.versioning", ) _EXPECTED = { "host.routes_legacy (pre-/admin view URL redirects)", @@ -36,6 +37,8 @@ "AuthProvider.get_public_paths", "register_event_handlers(self, bus) without the app parameter", "simple_module_inertia.props.lazy", + "ModuleBase.register_event_handlers app=None default", + "ModuleMeta.requires_framework=None compatibility bypass", } @@ -44,6 +47,14 @@ def _register_all() -> None: warnings.simplefilter("ignore", DeprecationWarning) for name in _SHIM_MODULES: importlib.import_module(name) + # Overrides own event-hook calls, so omission cannot warn reliably at the base. + from simple_module_core.module import EVENT_HANDLERS_OPTIONAL_APP_REMOVE_IN + + deprecate( + "ModuleBase.register_event_handlers app=None default", + remove_in=EVENT_HANDLERS_OPTIONAL_APP_REMOVE_IN, + use="register_event_handlers(self, bus, app) with an explicit app argument", + ) # simple_module_inertia is standalone (no core dependency), so it exposes a # constant instead of registering itself. from simple_module_inertia import props From 5b27972ff8b838d17f32b4fac02dd2a3277bda18 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 11:46:39 +0200 Subject: [PATCH 3/9] fix: resolve batch-5 merge in maintenance.py (drop unused logger) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/hosting/simple_module_hosting/maintenance.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/framework/hosting/simple_module_hosting/maintenance.py b/framework/hosting/simple_module_hosting/maintenance.py index 4b99ecb3b..2be0266cb 100644 --- a/framework/hosting/simple_module_hosting/maintenance.py +++ b/framework/hosting/simple_module_hosting/maintenance.py @@ -21,8 +21,6 @@ from simple_module_hosting._error_page import render_error_page from simple_module_hosting.public_access import is_public -logger = logging.getLogger(__name__) - # Kept reachable while the gate is closed: liveness probes (so orchestrators # do not kill the pod mid-maintenance), static assets and i18n bundles (or the # 503 page renders unstyled and untranslated). From ccbbeb5e4eca711d9fa9890267d9db41df41ae33 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 11:51:19 +0200 Subject: [PATCH 4/9] test: patch migration_status where the setup gate reads it Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/hosting/tests/test_hosting_error_handling_policy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/hosting/tests/test_hosting_error_handling_policy.py b/framework/hosting/tests/test_hosting_error_handling_policy.py index 50fded96c..5753c6075 100644 --- a/framework/hosting/tests/test_hosting_error_handling_policy.py +++ b/framework/hosting/tests/test_hosting_error_handling_policy.py @@ -59,7 +59,7 @@ def broken(request): async def test_migration_failure_warns_once_and_keeps_gate_closed(monkeypatch, caplog): monkeypatch.setattr(setup_gate, "_migration_failure_warned", False) check = AsyncMock(side_effect=RuntimeError("database unavailable")) - monkeypatch.setattr(setup_gate, "migration_status", check) + monkeypatch.setattr(setup_gate.migrations, "migration_status", check) snapshot = {"is_current": False} app = SimpleNamespace( state=SimpleNamespace( From b9102c85c3c31915db8934541d44b93440ea3dfc Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 11:57:20 +0200 Subject: [PATCH 5/9] fix: do not warn per module for the requires_framework=None bypass Every unversioned module (all in-repo ones) warned on each boot, flooding test runs with ~27k warnings; the bypass stays tracked by the deprecation-expiry test. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/simple_module_core/module_meta.py | 3 ++- framework/core/simple_module_core/versioning.py | 4 +++- framework/core/tests/test_versioning.py | 3 +-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/framework/core/simple_module_core/module_meta.py b/framework/core/simple_module_core/module_meta.py index 94ab87fe7..4ab7f2825 100644 --- a/framework/core/simple_module_core/module_meta.py +++ b/framework/core/simple_module_core/module_meta.py @@ -51,7 +51,8 @@ class ModuleMeta: Example: ``">=1.0,<2.0"``. When set, the module is rejected at boot if the installed ``simple_module_core.FRAMEWORK_API_VERSION`` does not satisfy it. When ``None``, no compatibility check is performed (legacy modules); - this deprecated bypass has ``remove_in="0.1.0"`` and warns at discovery. + this bypass is deprecated with ``remove_in="0.1.0"`` (tracked by the + deprecation-expiry test; it does not warn at boot). """ i18n_audience: str = "public" """Who this module's locale catalog is shipped to: ``"public"`` or ``"admin"``. diff --git a/framework/core/simple_module_core/versioning.py b/framework/core/simple_module_core/versioning.py index 223b6b3d7..ccb215a10 100644 --- a/framework/core/simple_module_core/versioning.py +++ b/framework/core/simple_module_core/versioning.py @@ -56,7 +56,9 @@ def check_framework_compatibility( for mod in modules: spec_str = mod.meta.requires_framework if spec_str is None: - _UNVERSIONED_MODULE.warn() + # Tracked for removal (``_UNVERSIONED_MODULE``) but not warned per + # module: every unversioned module, in-repo ones included, would + # warn on each boot. continue try: spec = SpecifierSet(spec_str) diff --git a/framework/core/tests/test_versioning.py b/framework/core/tests/test_versioning.py index 6aaaa2850..651819c6a 100644 --- a/framework/core/tests/test_versioning.py +++ b/framework/core/tests/test_versioning.py @@ -61,8 +61,7 @@ async def test_check_compat_skips_modules_without_spec(self): class ModLegacy(ModuleBase): meta = ModuleMeta(name="Legacy") - with pytest.warns(DeprecationWarning, match=r"requires_framework=None.*0\.1\.0"): - check_framework_compatibility([ModLegacy()]) + check_framework_compatibility([ModLegacy()]) async def test_check_compat_rejects_malformed_spec(self): """A malformed version specifier raises FrameworkVersionError (not something cryptic).""" From ba9a58237395c0750fd389c08bfa8b67358da76d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 11:57:37 +0200 Subject: [PATCH 6/9] changelog: batch 5 entries (#593 #594) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d93b2063c..4bf16b416 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,19 @@ All notable changes to this project are documented in this file. The format is b - `EventBus.publish_durable_or_inline(event, db)`: enqueue durably when a transport is installed, otherwise run the durable handlers inline in `db`'s transaction (#583). +### Changed +- Hosting: one `simple_module_hosting.public_access.is_public(app, method, path)` answers + "is this route public?" for `AuthMiddleware`, maintenance mode, rate limiting and the + `SM031` check, so they can no longer disagree. A legacy auth provider whose deprecated + `get_public_paths()` raises is now logged everywhere instead of silently treated as not + public in route access (#593). +- Framework: fail-open error handlers in the setup gate, DB listeners, menu providers, plural + rules and the invalidation decoder now log a WARNING (once) with the traceback instead of + failing silently; outcomes are unchanged. The policy is in `docs/framework-conventions.md`. + The test plugin resets the process-wide mail sender and lease backend between apps. The + `register_event_handlers(app=None)` default and the `requires_framework=None` bypass are + tracked for removal in 0.1.0 (#594). + ### Fixed - Tenants: deleting a tenant now tears down its data. `TenantDeleted` (now defined in `simple_module_core.tenant_events`, still importable from `tenants.contracts.events`) is From 13e01e40bd284a51445ee0a9c0337bc0e1bbc927 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 12:00:59 +0200 Subject: [PATCH 7/9] fix: address code review findings (round 1, pass 1) A failing legacy auth provider is logged once per process instead of per request. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/hosting/simple_module_hosting/public_access.py | 8 +++++++- framework/hosting/tests/test_hosting_public_access.py | 5 ++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/framework/hosting/simple_module_hosting/public_access.py b/framework/hosting/simple_module_hosting/public_access.py index 4aee11d58..839beebef 100644 --- a/framework/hosting/simple_module_hosting/public_access.py +++ b/framework/hosting/simple_module_hosting/public_access.py @@ -8,6 +8,7 @@ from simple_module_core.public_routes import legacy_provider_public_paths logger = logging.getLogger(__name__) +_provider_failure_warned = False def is_public(app: FastAPI, method: str, path: str) -> bool: @@ -21,6 +22,11 @@ def is_public(app: FastAPI, method: str, path: str) -> bool: try: prefixes, exact = legacy_provider_public_paths(provider) except Exception: - logger.exception("Auth provider failed to report public paths") + # Runs per request (and per route x placeholder in SM031): warn once per + # process, per the error-handling policy, instead of a traceback each time. + global _provider_failure_warned + if not _provider_failure_warned: + _provider_failure_warned = True + logger.warning("Auth provider failed to report public paths", exc_info=True) return False return any(path.startswith(prefix) for prefix in prefixes) or path in exact diff --git a/framework/hosting/tests/test_hosting_public_access.py b/framework/hosting/tests/test_hosting_public_access.py index 41505f078..fc4afde52 100644 --- a/framework/hosting/tests/test_hosting_public_access.py +++ b/framework/hosting/tests/test_hosting_public_access.py @@ -5,6 +5,7 @@ import pytest from fastapi import FastAPI from simple_module_core.public_routes import PublicRouteRegistry +from simple_module_hosting import public_access from simple_module_hosting._route_access import is_public_operation from simple_module_hosting.public_access import is_public @@ -40,7 +41,9 @@ def get_public_paths(self): @pytest.mark.parametrize("operation", [False, True]) -def test_raising_legacy_provider_is_logged(caplog, operation: bool) -> None: +def test_raising_legacy_provider_is_logged(caplog, monkeypatch, operation: bool) -> None: + monkeypatch.setattr(public_access, "_provider_failure_warned", False) + class RaisingProvider: def get_public_paths(self): raise RuntimeError("provider failed") From 598fee9adb257a303017bf893adc217d28c45e34 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 12:02:09 +0200 Subject: [PATCH 8/9] fix: address code review findings (round 1, pass 1 follow-up) Process state resets stay per-test (a second app no longer clobbers the first); auth declares its simple_module_hosting dependency. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/testing/simple_module_test/fixtures.py | 6 +++--- modules/auth/pyproject.toml | 2 ++ uv.lock | 4 +++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/framework/testing/simple_module_test/fixtures.py b/framework/testing/simple_module_test/fixtures.py index 53029b13c..569474f2f 100644 --- a/framework/testing/simple_module_test/fixtures.py +++ b/framework/testing/simple_module_test/fixtures.py @@ -161,9 +161,9 @@ async def _seed_setup_admin(application) -> None: async def _build_app(settings: Settings, *, seed_admin: bool): from simple_module_hosting.app_builder import create_app - from simple_module_test.process_state import reset_process_state - - reset_process_state() + # Process-wide state is reset per test by the autouse fixture in + # ``process_state``, not here: a second app built inside one test must not + # reset the live first app's mail sender or lease backend. application = create_app(settings) await _create_all_tables(application.state.sm.db.engine) diff --git a/modules/auth/pyproject.toml b/modules/auth/pyproject.toml index b8a3f32c6..1745d5365 100644 --- a/modules/auth/pyproject.toml +++ b/modules/auth/pyproject.toml @@ -24,6 +24,7 @@ dependencies = [ "itsdangerous>=2.2", "simple_module_core==0.0.38", "simple_module_db==0.0.38", + "simple_module_hosting==0.0.38", ] [project.entry-points.simple_module] @@ -50,3 +51,4 @@ packages = ["auth"] [tool.uv.sources] simple_module_core = { workspace = true } simple_module_db = { workspace = true } +simple_module_hosting = { workspace = true } diff --git a/uv.lock b/uv.lock index 8ab30fa39..2d47c31f7 100644 --- a/uv.lock +++ b/uv.lock @@ -1,5 +1,5 @@ version = 1 -revision = 5 +revision = 3 requires-python = ">=3.12" resolution-markers = [ "python_full_version >= '3.14'", @@ -3104,6 +3104,7 @@ dependencies = [ { name = "itsdangerous" }, { name = "simple-module-core" }, { name = "simple-module-db" }, + { name = "simple-module-hosting" }, ] [package.metadata] @@ -3111,6 +3112,7 @@ requires-dist = [ { name = "itsdangerous", specifier = ">=2.2" }, { name = "simple-module-core", editable = "framework/core" }, { name = "simple-module-db", editable = "framework/db" }, + { name = "simple-module-hosting", editable = "framework/hosting" }, ] [[package]] From 224728cfe13fd353c8e39998c7cb3bc16fe75142 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 23 Oct 2026 12:03:17 +0200 Subject: [PATCH 9/9] test: pin per-test process-state reset and two-app safety Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tests/test_testing_process_state.py | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/framework/testing/tests/test_testing_process_state.py b/framework/testing/tests/test_testing_process_state.py index 4ea442716..d20f47e6e 100644 --- a/framework/testing/tests/test_testing_process_state.py +++ b/framework/testing/tests/test_testing_process_state.py @@ -65,18 +65,16 @@ def test_reset_sentry_closes_and_unbinds_client(monkeypatch) -> None: scope.client.close.assert_called_once_with() -async def test_plugin_app_builder_resets_services_between_apps(settings, monkeypatch) -> None: +async def test_plugin_second_app_keeps_first_apps_services(settings, monkeypatch) -> None: + """Resets are per test (autouse fixture), so a second app in one test must not + wipe the first app's installed mail sender.""" from fastapi import FastAPI from simple_module_hosting import app_builder from simple_module_test import fixtures - seen = [] - def create_app(settings): - seen.append((mail_transport_installed(), get_leases(), sentry.is_enabled())) app = FastAPI() app.state.sm = Mock() - install_mail_sender(CapturedMail()) return app async def create_tables(engine): @@ -84,10 +82,13 @@ async def create_tables(engine): monkeypatch.setattr(app_builder, "create_app", create_app) monkeypatch.setattr(fixtures, "_create_all_tables", create_tables) - for _ in range(2): - async for _app in fixtures._build_app(settings, seed_admin=False): - pass - assert len(seen) == 2 - assert all(not mail and not enabled for mail, _, enabled in seen) - assert seen[0][1] is not seen[1][1] + install_mail_sender(CapturedMail()) + async for _app in fixtures._build_app(settings, seed_admin=False): + assert mail_transport_installed() reset_process_state() + + +def test_autouse_reset_clears_state_left_by_a_previous_test() -> None: + # The autouse ``_reset_process_state`` fixture ran before this test. + assert not mail_transport_installed() + assert not sentry.is_enabled()