diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f571c10..4c80f5d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,6 +114,23 @@ All notable changes to this project are documented in this file. The format is b module defers past the request (#364). The `tenants` module resolves a tenant from the subdomain (`subdomain_base`), anonymous visitors included (#363). +- **Request-path performance** (Postgres load test, + [docs/perf/2026-10-08-postgres-loadtest.md](docs/perf/2026-10-08-postgres-loadtest.md)). + Route matching skips any included router whose routes cannot match the path: + FastAPI ≥ 0.140 regex-tested nearly all ~190 routes per request, ~30% of a + cheap request's CPU (`/health` 1.98 → 1.46 ms). The guards are built at + startup, which also takes FastAPI's lazy per-route build off the first + request after boot (360 → 32 ms). GZip runs at level 5 instead of 9: about + 1% larger output for 2.4× less CPU. Trivial sync FastAPI dependencies + (`get_permission_registry`, `get_feature_flag_registry`, the `file_storage`, + `tenants` and `users` accessors) are now `async`, so they no longer + take a threadpool round-trip. `/admin/users` counts its status cards in one + query instead of three. +- `SetupMiddleware` refreshes its cached verdict single-flight. When the 5 s + TTL lapsed under load, every in-flight request ran the setup steps itself, + each checking out a pooled connection. Behind a saturated pool those + checkouts queued and fed `QueuePool limit … reached` timeouts. The expired + *complete* verdict now keeps answering while one refresh runs. ### Security - The tenant header (`tenant_header`) is no longer honoured for an diff --git a/docs/framework/middleware.md b/docs/framework/middleware.md index 5ca40121..fc2093e9 100644 --- a/docs/framework/middleware.md +++ b/docs/framework/middleware.md @@ -138,6 +138,8 @@ Emits a structured log line per request with method, path, status, duration, and Starlette's, compressing any response body over `COMPRESSION_MIN_BYTES` (500). Placed inside `CorrelationId` and `RequestLogging` — which set headers and read request state — but outside everything that produces a body, **including the `/static` mount**, which is where it earns its place: the built CSS is ~139 KB raw against ~21 KB gzipped, and the JS bundle compresses about 3×. Uncompressed assets dominated cold page load, several times larger than anything on the server request path. +It runs at `COMPRESSION_LEVEL` 5, not Starlette's default of 9. Compression runs on the event loop that serves every other request, and level 9 buys almost nothing on JSON and HTML: a 76 KB first-load page comes out 1.2% smaller than at level 5 for 2.4× the CPU (6.4 ms vs 2.7 ms). Pre-compressed static assets are served as-is and never re-compressed, so they keep whatever level they were built with. + ### `SecurityHeadersMiddleware` Sets conservative defaults: `X-Content-Type-Options: nosniff`, `Referrer-Policy: strict-origin-when-cross-origin`, `X-Frame-Options: SAMEORIGIN`, `X-XSS-Protection: 0` (the legacy auditor is disabled in favour of CSP), plus a default CSP and — outside development — HSTS. In development the CSP is widened for the Vite dev origin and HSTS is suppressed. Modules that load assets from an external origin extend the policy through the [`register_csp_sources`](lifecycle.md#register_csp_sourcesregistry) hook; both the dev and production variants honor those origins. Override on a per-route basis with your own response headers. diff --git a/docs/perf/2026-10-08-postgres-loadtest.md b/docs/perf/2026-10-08-postgres-loadtest.md new file mode 100644 index 00000000..c63b7224 --- /dev/null +++ b/docs/perf/2026-10-08-postgres-loadtest.md @@ -0,0 +1,109 @@ +# Backend load test on Postgres + +**Date:** 2026-10-08 +**Harness:** [tests/loadtest/](../../tests/loadtest/README.md) (locust + faker seed) + +## Environment + +| | | +|---|---| +| Machine | linux, 24 cores, shared `dev-services` Postgres (Docker, `max_connections=100`) | +| Database | Postgres, fresh DB, 10 000 users (9 000 role assignments), 100 000 audit rows, `ANALYZE`d | +| Modules | `Auth, FeatureFlags, Settings, FileStorage, Users, AuditLog, BackgroundTasks, Dashboard, Permissions, Tenants, SiteLock, Branding` | +| Server | `uvicorn host.main:app`, development mode, one worker unless stated | +| Load | `locustfile.py` `AuthedUser` mix, 300 users, spawn 100/s, 60 s | +| Micro | in-process `httpx.ASGITransport`, sequential, 200 warm requests per URL, median | + +## Saturated throughput (300 users, 1 worker) + +| | req/s | p50 | p95 | failures | +|---|---:|---:|---:|---:| +| Before | 117.7 | 330 ms | 9.7 s | 1 (`QueuePool limit … reached`) | +| After | 123.5 | 330 ms | 9.5 s | 3 | +| After, `SM_DB_POOL_PRE_PING=false` | 127.1 | 310 ms | 9.8 s | 1 | +| After, 4 workers, pool 5 + 10 | **520.9** | **180 ms** | **790 ms** | **0** | + +One worker is **CPU-bound**: the process sat at 98% of one core while +Postgres used about one core. The p95 of ~10 s is queueing, not slow +queries. Weighting each endpoint's measured CPU by the locust mix gives +~8.7 ms per request, a ceiling of ~115–125 req/s on one core, which matches what +was observed. Behind a saturated event loop each request holds its pooled +connection far longer than its queries take, so the pool (10 + 20) runs dry +and requests wait up to `pool_timeout` (30 s). + +So throughput is a deployment lever, as in the June campaign: four workers +with the pool cut so that `workers × (pool_size + max_overflow)` stays under +`max_connections` gave 4.4× the throughput and a 12× better p95 with no +errors. See `docs/reference/deployment.md`. + +## Per-request cost (in-process, ms, median) + +Wall time; the DB-backed rows include Postgres round-trips and are noisy on the +shared instance. + +| URL | before | after | +|---|---:|---:| +| `/health` | 1.98 | 1.46 | +| `/api/feature_flags/` | 2.19 | 1.90 | +| `/api/permissions/` | 3.00 | 2.52 | +| `/api/dashboard/stats` | 2.53 | 2.12 | +| `/dashboard/` (Inertia) | 3.67 | 3.24 | +| `/admin/users/` (Inertia) | 28.9 | 25.3 | +| `/api/users/admin` | 11.2 | 11.0 | +| `/api/audit_log/` | 22.0 | 24.1 | +| `/api/settings/modules` | 8.58 | 8.13 | + +Main-thread CPU for the heavy endpoints: `/admin/users/` 17.5 ms, +`/api/users/admin` 10.5 ms, `/api/audit_log/` 8.4 ms, `/api/settings/modules` +8.1 ms. These make up most of the mix's CPU. + +## Fixed + +1. **Route matching was ~30% of a cheap request's CPU.** FastAPI ≥ 0.140 + keeps every `include_router` as a lazy `_IncludedRouter`. The app root + holds ~35 of them (one API and one view router per module), and none + filters on its prefix, so a request regex-tested nearly all ~190 routes. It + also re-walked each subtree's route version and ran `_match` twice on the + router that matched. `simple_module_hosting._route_guard` gives each + top-level include a prefix guard derived from its effective route paths and + re-derived when FastAPI's route version changes. Saves ~0.5 ms per request. +2. **Cold start.** The first request to reach a router builds every one of its + routes' dependants (signatures and pydantic adapters). The guards are built + at lifespan start, which does that work during boot: the first request + dropped from 360 ms to 32 ms, and startup grew by about the same amount. +3. **GZip at level 9.** Starlette's default. A 76 KB first-load page cost + 6.4 ms of event-loop CPU at level 9 against 2.7 ms at level 5, for 1.2% + less output. Now level 5. +4. **Setup-gate stampede.** The verdict TTL (5 s) lapsing under load made + every in-flight request run `has_administrator` on its own pooled + connection, which fed the pool exhaustion. Now single-flight, and an + expired *complete* verdict answers while the refresh runs. +5. **Threadpool hops.** Trivial sync dependency getters made FastAPI dispatch + each one to the threadpool. They are now `async def`. +6. **`/admin/users/` status cards** issued three `COUNT(*)` round-trips; now + one `COUNT(*) FILTER (WHERE …)` scan. + +## Not changed: findings and follow-ups + +- **`pool_pre_ping`** runs `BEGIN; ; ROLLBACK` (three round-trips) on every + checkout. That is ~1.4 ms of held-connection time per DB request against a + 331 µs bare round-trip, plus ~4.5% of request CPU. Turning it off gave +3% + throughput. It stays on by default, since it is what lets a pool survive a + database restart, but a host with a stable database and its own retry layer + can set `SM_DB_POOL_PRE_PING=false`. +- **Soft-delete/tenant statement filter** (`query_filter.filter_statements`) + attaches `with_loader_criteria` for every soft-delete model to every SELECT + and walks subqueries: ~0.35 ms of CPU per statement. Compiled-cache hits + are unaffected (every statement measured was a `CACHE_HIT`). It is the + isolation boundary from #332, so it needs a design-level review rather than + a perf patch. +- **Audit-log `COUNT(*)`** over 100k rows takes ~15 ms on this instance and + dominates `/api/audit_log/`. An estimated or capped count would be a UX + decision. +- **`/api/settings/modules`** spends ~8 ms rebuilding the view of ~110 + settings fields per call. It is a low-traffic admin screen that the mix + over-weights, and its output depends on live env, DB overrides and secret + masking, so it is left uncached. +- Inertia payloads: the first navigation per session carries the full i18n + catalog (~75 KB of a 76 KB page); later navigations send 4.3 KB. A client + without a cookie jar (curl) sees 76 KB every time, so measure with one. diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index 8e623d11..f86b0ea8 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -20,6 +20,7 @@ from fastapi import FastAPI +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 @@ -171,6 +172,9 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: exc_info=True, ) app.state.deferred_startup.append(mod) + # Here rather than in create_app: the host includes its own routers + # after create_app returns, and those should be guarded too. + guard_included_routers(app) yield for mod in reversed(modules): await mod.on_shutdown(app) diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index fb66a47e..e5e1602d 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -58,6 +58,11 @@ # Below this, gzip framing costs more than it saves. Starlette's own default. COMPRESSION_MIN_BYTES = 500 +# Starlette defaults to 9, which buys almost nothing on JSON and HTML: a 76 KB +# first-load page compresses 1.2% smaller at 9 than at 5, for 2.4x the CPU +# (6.4 ms vs 2.7 ms), spent on the event loop that serves every other request. +COMPRESSION_LEVEL = 5 + # Re-exported for back-compat: static-file serving now lives in static_files. ImmutableStaticFiles = PrecompressedStaticFiles @@ -176,7 +181,9 @@ def install_middleware( # matters most: the built CSS is ~139 KB raw and ~21 KB gzipped, and the # JS bundle compresses about 3x. Uncompressed assets dominated cold page # load, several times larger than anything on the server request path. - app.add_middleware(GZipMiddleware, minimum_size=COMPRESSION_MIN_BYTES) + app.add_middleware( + GZipMiddleware, minimum_size=COMPRESSION_MIN_BYTES, compresslevel=COMPRESSION_LEVEL + ) # Right inside RequestLogging (so a 413 is logged) and outside GZip and the # whole module tier: an oversized body is refused before anything reads it. app.add_middleware( diff --git a/framework/hosting/simple_module_hosting/_route_guard.py b/framework/hosting/simple_module_hosting/_route_guard.py new file mode 100644 index 00000000..4d1ffe58 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_route_guard.py @@ -0,0 +1,98 @@ +"""Skip whole included routers whose routes cannot match the request path. + +FastAPI >= 0.140 keeps every ``include_router`` as a lazy ``_IncludedRouter`` +placeholder instead of copying its routes onto the parent. Matching a request +then asks each placeholder in turn, and a placeholder that cannot match still +walks its subtree's route version and regex-tests every route in it. With one +API and one view router per module the app root holds ~35 placeholders, so a +request for a module near the end of the list tested nearly all ~190 routes: +route matching was ~30% of the CPU of a cheap authenticated request. + +Every route under a placeholder shares the common prefix of its effective +paths. A request whose path lacks that prefix cannot match any of them — +neither fully nor partially — so the guard answers ``Match.NONE`` without +descending. Anything else is delegated unchanged, so a match still takes +FastAPI's own path. + +The prefix is derived from the routes, not from ``APIRouter.prefix``: a route +added with ``add_route`` does not carry its router's prefix. It is recomputed +whenever FastAPI's route version for that router changes, so a router mutated +after the app was built is never filtered against a stale prefix. + +Relies on ``fastapi.routing._IncludedRouter``; on a FastAPI without it (which +flattened routes eagerly and needs no guard) this is a no-op. +""" + +from __future__ import annotations + +import os +from typing import Any + +from fastapi import FastAPI +from starlette._utils import get_route_path +from starlette.routing import Match +from starlette.types import Scope + +try: # private, and absent before FastAPI 0.140 + from fastapi.routing import _IncludedRouter +except ImportError: # pragma: no cover - older FastAPI flattens eagerly + _IncludedRouter = None # type: ignore[assignment,misc] + +__all__ = ["guard_included_routers"] + + +def _effective_paths(included: Any) -> list[str]: + paths: list[str] = [] + for ctx in included.effective_route_contexts(): + route = ctx.starlette_route + paths.append((getattr(route, "path", "") if route is not None else ctx.path) or "") + return paths + + +def _common_prefix(paths: list[str]) -> str: + """The literal leading part every path shares — never past a ``{param}``.""" + if not paths: + return "" + return os.path.commonprefix(paths).split("{", 1)[0] + + +class _PrefixGuard: + """Replacement ``matches`` for one ``_IncludedRouter`` instance.""" + + def __init__(self, included: Any) -> None: + self._included = included + self._matches = included.matches + self._version: int | None = None + self._prefix = "" + + def _current_prefix(self) -> str: + version = self._included.original_router._get_routes_version() + if version != self._version: + self._prefix = _common_prefix(_effective_paths(self._included)) + self._version = version + return self._prefix + + def __call__(self, scope: Scope) -> tuple[Match, Scope]: + prefix = self._current_prefix() + # "/" (or "") is shared by every path, so it would filter nothing. + if len(prefix) > 1 and not get_route_path(scope).startswith(prefix): + return Match.NONE, {} + return self._matches(scope) + + +def guard_included_routers(app: FastAPI) -> None: + """Install a prefix guard on each router included directly into ``app``. + + Computing a guard's prefix resolves its router's effective routes, which is + also what FastAPI does lazily on the first request to reach them — building + every route's dependant, signature and pydantic adapters. Doing it here + moves that one-time cost (hundreds of ms for a full module set) from the + first request after boot into startup. + """ + if _IncludedRouter is None: + return + for route in app.router.routes: + if isinstance(route, _IncludedRouter) and not isinstance(route.matches, _PrefixGuard): + guard = _PrefixGuard(route) + guard._current_prefix() + route.matches = guard # type: ignore[method-assign] diff --git a/framework/hosting/simple_module_hosting/setup_gate.py b/framework/hosting/simple_module_hosting/setup_gate.py index d5e71258..676ff24e 100644 --- a/framework/hosting/simple_module_hosting/setup_gate.py +++ b/framework/hosting/simple_module_hosting/setup_gate.py @@ -12,6 +12,7 @@ from __future__ import annotations +import asyncio import logging import time @@ -128,6 +129,7 @@ def __init__(self, app: ASGIApp) -> None: self.app = app self._verdict: bool | None = None self._verdict_expires: float = 0.0 + self._refresh: asyncio.Future[bool] | None = None self._announced = False async def _is_complete(self, registry, starlette_app) -> bool: @@ -149,15 +151,39 @@ async def _is_complete(self, registry, starlette_app) -> bool: The cache lives on the middleware instance rather than on ``app.state`` so it cannot leak between two apps built in the same process — the test suite builds many — and is discarded with the app that owns it. + + Refreshes are single-flight. When the TTL lapses under load, every + request in flight used to run its own evaluation, each checking out a + pooled connection; behind a saturated pool those checkouts queued for + seconds and starved the requests that actually needed the database. + Now one evaluation runs, and while it does, the expired *complete* + verdict keeps answering — the refresh still lands every TTL, so a lost + administrator still brings the wizard back. With no complete verdict to + fall back on, callers share the in-flight evaluation instead. """ now = time.monotonic() if self._verdict and now < self._verdict_expires: return True - verdict = await registry.is_setup_complete(starlette_app) - if verdict: + refresh = self._refresh + if refresh is None: + refresh = asyncio.ensure_future(registry.is_setup_complete(starlette_app)) + refresh.add_done_callback(self._refresh_done) + self._refresh = refresh + elif self._verdict: + return True + # Shielded so a caller that is cancelled (client gone) does not cancel + # the evaluation every other waiter is sharing. + return await asyncio.shield(refresh) + + def _refresh_done(self, refresh: asyncio.Future) -> None: + self._refresh = None + if refresh.cancelled() or refresh.exception() is not None: + return + if refresh.result(): self._verdict = True - self._verdict_expires = now + _VERDICT_TTL_SECONDS - return verdict + self._verdict_expires = time.monotonic() + _VERDICT_TTL_SECONDS + else: + self._verdict = False async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: if scope["type"] != "http": diff --git a/framework/hosting/tests/test_route_guard.py b/framework/hosting/tests/test_route_guard.py new file mode 100644 index 00000000..06587e54 --- /dev/null +++ b/framework/hosting/tests/test_route_guard.py @@ -0,0 +1,131 @@ +"""guard_included_routers: skip included routers that cannot match the path. + +The guard is a pure optimisation, so most of these pin that routing answers +exactly as it did unguarded; the last one pins that it actually skips. +""" + +from __future__ import annotations + +import httpx +import pytest +from fastapi import APIRouter, FastAPI +from fastapi.routing import _IncludedRouter +from simple_module_hosting._route_guard import guard_included_routers +from starlette.responses import PlainTextResponse + + +def _app() -> tuple[FastAPI, APIRouter]: + app = FastAPI() + alpha = APIRouter(prefix="/api/alpha") + + @alpha.get("/items/{item_id}") + async def item(item_id: int) -> dict: + return {"item": item_id} + + @alpha.get("/") + async def alpha_index() -> dict: + return {"index": "alpha"} + + nested = APIRouter(prefix="/nested") + + @nested.get("/deep") + async def deep() -> dict: + return {"deep": True} + + alpha.include_router(nested) + + beta = APIRouter(prefix="/api/beta") + + @beta.get("/ping") + async def ping() -> dict: + return {"beta": "pong"} + + app.include_router(alpha) + app.include_router(beta) + return app, alpha + + +async def _get(app: FastAPI, path: str) -> httpx.Response: + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://t") as client: + return await client.get(path) + + +@pytest.mark.parametrize( + ("path", "status", "body"), + [ + ("/api/alpha/items/7", 200, {"item": 7}), + ("/api/alpha/", 200, {"index": "alpha"}), + ("/api/alpha/nested/deep", 200, {"deep": True}), + ("/api/beta/ping", 200, {"beta": "pong"}), + ("/api/gamma/ping", 404, None), + ("/api/alpha/items/not-an-int", 422, None), + ], +) +async def test_routing_is_unchanged(path: str, status: int, body: dict | None) -> None: + app, _ = _app() + guard_included_routers(app) + response = await _get(app, path) + assert response.status_code == status + if body is not None: + assert response.json() == body + + +async def test_trailing_slash_redirect_still_fires() -> None: + """``/api/alpha`` lacks the guard's ``/api/alpha/`` prefix; the redirect + pass retries with the slash added, which the guard lets through.""" + app, _ = _app() + guard_included_routers(app) + response = await _get(app, "/api/alpha") + assert response.status_code == 307 + assert response.headers["location"].endswith("/api/alpha/") + + +async def test_a_router_mutated_after_guarding_is_not_filtered_stale() -> None: + """``add_route`` does not apply the router prefix, so the new route sits + outside the prefix computed at guard time; it must still be reachable.""" + app, alpha = _app() + guard_included_routers(app) + assert (await _get(app, "/api/alpha/items/1")).status_code == 200 # guard primed + + async def outside(_request): + return PlainTextResponse("outside") + + alpha.add_route("/elsewhere", outside) + + response = await _get(app, "/elsewhere") + assert response.status_code == 200 + assert response.text == "outside" + + +async def test_guarding_twice_is_a_no_op() -> None: + app, _ = _app() + guard_included_routers(app) + guarded = [r.matches for r in app.router.routes if isinstance(r, _IncludedRouter)] + guard_included_routers(app) + assert [r.matches for r in app.router.routes if isinstance(r, _IncludedRouter)] == guarded + assert (await _get(app, "/api/beta/ping")).status_code == 200 + + +async def test_a_non_matching_router_is_not_descended() -> None: + app, alpha = _app() + guard_included_routers(app) + descended = 0 + alpha_matches = alpha.matches + + def spy(scope): + nonlocal descended + descended += 1 + return alpha_matches(scope) + + alpha.matches = spy # type: ignore[method-assign] + assert (await _get(app, "/api/beta/ping")).status_code == 200 + assert descended == 0, "the guard let a request for /api/beta into /api/alpha's routes" + + +async def test_the_app_lifespan_guards_every_top_level_include(app) -> None: + from simple_module_hosting._route_guard import _PrefixGuard + + included = [r for r in app.router.routes if isinstance(r, _IncludedRouter)] + assert included + assert all(isinstance(r.matches, _PrefixGuard) for r in included) diff --git a/framework/hosting/tests/test_setup_gate_single_flight.py b/framework/hosting/tests/test_setup_gate_single_flight.py new file mode 100644 index 00000000..8d78ae8e --- /dev/null +++ b/framework/hosting/tests/test_setup_gate_single_flight.py @@ -0,0 +1,137 @@ +"""SetupMiddleware refreshes its verdict single-flight. + +Under load, every request that found the cached verdict expired used to run the +setup steps itself — each a session checkout and a ``COUNT(*)`` — and behind a +saturated pool those queued for seconds and starved real work. See +``SetupMiddleware._is_complete``; the TTL/caching rules are in +``test_setup_gate``. +""" + +from __future__ import annotations + +import asyncio +from types import SimpleNamespace + +from simple_module_core.setup_steps import SetupRegistry, SetupStep + + +def _scope() -> dict: + return {"type": "http", "path": "/dashboard", "method": "GET", "headers": [], "app": None} + + +async def _status(middleware, registry) -> int: + scope = _scope() + sm = SimpleNamespace(setup_registry=registry) + scope["app"] = SimpleNamespace(state=SimpleNamespace(sm=sm)) + sent: dict = {} + + async def send(message): + if message["type"] == "http.response.start": + sent.update(message) + + async def receive(): + return {"type": "http.request", "body": b"", "more_body": False} + + await middleware(scope, receive, send) + return sent["status"] + + +async def _passthrough(scope, receive, send): + await send({"type": "http.response.start", "status": 200, "headers": []}) + await send({"type": "http.response.body", "body": b"ok"}) + + +class _GatedStep: + """A step whose evaluation blocks until released, counting evaluations.""" + + def __init__(self, result: bool) -> None: + self.result = result + self.calls = 0 + self.release = asyncio.Event() + + async def __call__(self, _app) -> bool: + self.calls += 1 + await self.release.wait() + return self.result + + def registry(self) -> SetupRegistry: + registry = SetupRegistry() + registry.add(SetupStep(id="users.administrator", title="admin", is_complete=self)) + return registry + + +async def test_concurrent_requests_share_one_evaluation() -> None: + from simple_module_hosting.setup_gate import SetupMiddleware + + step = _GatedStep(result=True) + registry = step.registry() + middleware = SetupMiddleware(_passthrough) + + pending = [asyncio.ensure_future(_status(middleware, registry)) for _ in range(20)] + await asyncio.sleep(0) + step.release.set() + + assert await asyncio.gather(*pending) == [200] * 20 + assert step.calls == 1, f"each waiting request ran the steps itself ({step.calls}x)" + + +async def test_an_expired_complete_verdict_answers_while_it_refreshes() -> None: + """Requests must not queue behind the refresh's database checkout.""" + from simple_module_hosting.setup_gate import SetupMiddleware + + step = _GatedStep(result=True) + step.release.set() + registry = step.registry() + middleware = SetupMiddleware(_passthrough) + assert await _status(middleware, registry) == 200 + + middleware._verdict_expires = 0.0 # the TTL lapses + step.release.clear() + refreshing = asyncio.ensure_future(_status(middleware, registry)) + for _ in range(10): # let the refresh start and block + if step.calls == 2: + break + await asyncio.sleep(0) + assert step.calls == 2 + + # Answered from the stale verdict, without waiting for the blocked refresh. + assert await asyncio.wait_for(_status(middleware, registry), timeout=1) == 200 + assert step.calls == 2 + + step.release.set() + assert await refreshing == 200 + + +async def test_a_refresh_that_finds_setup_incomplete_brings_the_wizard_back() -> None: + """Serving stale while refreshing must not hide a lost administrator.""" + from simple_module_hosting.setup_gate import SetupMiddleware + + step = _GatedStep(result=True) + step.release.set() + registry = step.registry() + middleware = SetupMiddleware(_passthrough) + assert await _status(middleware, registry) == 200 + + middleware._verdict_expires = 0.0 + step.result = False # the last administrator was deactivated + + assert await _status(middleware, registry) == 302 + assert await _status(middleware, registry) == 302 + + +async def test_a_cancelled_caller_does_not_cancel_the_shared_refresh() -> None: + from simple_module_hosting.setup_gate import SetupMiddleware + + step = _GatedStep(result=True) + registry = step.registry() + middleware = SetupMiddleware(_passthrough) + + first = asyncio.ensure_future(_status(middleware, registry)) + second = asyncio.ensure_future(_status(middleware, registry)) + await asyncio.sleep(0) + first.cancel() # the client that started the refresh went away + await asyncio.sleep(0) + step.release.set() + + assert await second == 200 + assert step.calls == 1 diff --git a/modules/dashboard/dashboard/endpoints/views.py b/modules/dashboard/dashboard/endpoints/views.py index 732bbd4e..31792a83 100644 --- a/modules/dashboard/dashboard/endpoints/views.py +++ b/modules/dashboard/dashboard/endpoints/views.py @@ -24,7 +24,7 @@ router = APIRouter() -def _require_admin(request: Request) -> None: +async def _require_admin(request: Request) -> None: """Doctor exposes migration status, module list and system info — admin only. The ``/admin`` prefix is a URL convention, not a permission, so this is the guard that actually keeps a non-admin, signed-in user out.""" diff --git a/modules/feature_flags/feature_flags/deps.py b/modules/feature_flags/feature_flags/deps.py index 8d928b96..b2ee8d9f 100644 --- a/modules/feature_flags/feature_flags/deps.py +++ b/modules/feature_flags/feature_flags/deps.py @@ -19,7 +19,7 @@ async def get_feature_flag_service( return FeatureFlagService(db) -def get_feature_flag_registry(request: Request) -> FeatureFlagRegistry: +async def get_feature_flag_registry(request: Request) -> FeatureFlagRegistry: """Return the process-wide FeatureFlagRegistry owned by the framework.""" return request.app.state.sm.feature_flags diff --git a/modules/file_storage/file_storage/deps.py b/modules/file_storage/file_storage/deps.py index 953751d9..6259e1bd 100644 --- a/modules/file_storage/file_storage/deps.py +++ b/modules/file_storage/file_storage/deps.py @@ -11,7 +11,7 @@ from file_storage.services import FileStorageServices -def get_file_storage_services(request: Request) -> FileStorageServices: +async def get_file_storage_services(request: Request) -> FileStorageServices: return request.app.state.file_storage @@ -22,5 +22,5 @@ async def get_file_storage_service( return FileStorageService(db, services.backend, services.settings, services.aggregates) -def get_event_bus(request: Request) -> EventBus: +async def get_event_bus(request: Request) -> EventBus: return request.app.state.sm.event_bus diff --git a/modules/permissions/permissions/deps.py b/modules/permissions/permissions/deps.py index fc9f7d30..5447e796 100644 --- a/modules/permissions/permissions/deps.py +++ b/modules/permissions/permissions/deps.py @@ -20,7 +20,7 @@ ] -def get_permission_registry(request: Request) -> PermissionRegistry: +async def get_permission_registry(request: Request) -> PermissionRegistry: return request.app.state.sm.permissions diff --git a/modules/tenants/tenants/deps.py b/modules/tenants/tenants/deps.py index 93fe210f..1a9d1633 100644 --- a/modules/tenants/tenants/deps.py +++ b/modules/tenants/tenants/deps.py @@ -16,7 +16,7 @@ from tenants.service import TenantService -def get_tenant_service(request: Request, db: AsyncSession = Depends(get_db)) -> TenantService: +async def get_tenant_service(request: Request, db: AsyncSession = Depends(get_db)) -> TenantService: app = request.app return TenantService( db, @@ -29,7 +29,7 @@ def get_tenant_service(request: Request, db: AsyncSession = Depends(get_db)) -> TenantServiceDep = Annotated[TenantService, Depends(get_tenant_service)] -def get_invitation_service(request: Request, tenants: TenantServiceDep) -> InvitationService: +async def get_invitation_service(request: Request, tenants: TenantServiceDep) -> InvitationService: ttl = request.app.state.tenants.settings.invitation_ttl_hours return InvitationService(tenants, ttl_hours=ttl) @@ -44,17 +44,21 @@ class ActiveTenantContext: user_id: str -def require_user_id(request: Request) -> str: +def _current_user_id(request: Request) -> str: user = getattr(request.state, "user", None) if user is None: raise TenantError("not_authenticated", status_code=401) return str(user.id) -def require_active_tenant(request: Request) -> ActiveTenantContext: +async def require_user_id(request: Request) -> str: + return _current_user_id(request) + + +async def require_active_tenant(request: Request) -> ActiveTenantContext: """The tenant resolved for this request. Operations act on it — never on an id from the URL — so a tenant-level permission cannot reach another tenant.""" - user_id = require_user_id(request) + user_id = _current_user_id(request) tenant_id = getattr(request.state, "tenant_id", None) role = getattr(request.state, "tenant_role", None) if tenant_id is None or role is None: @@ -62,7 +66,7 @@ def require_active_tenant(request: Request) -> ActiveTenantContext: return ActiveTenantContext(tenant_id=tenant_id, role=role, user_id=user_id) -def require_tenant_manager( +async def require_tenant_manager( ctx: Annotated[ActiveTenantContext, Depends(require_active_tenant)], ) -> ActiveTenantContext: """An owner or admin *of the active tenant*. diff --git a/modules/users/users/admin/queries.py b/modules/users/users/admin/queries.py index 2a2966f7..a8a9cfad 100644 --- a/modules/users/users/admin/queries.py +++ b/modules/users/users/admin/queries.py @@ -226,16 +226,16 @@ async def count_user_states(self) -> dict[str, int]: deck labels "Pending invites" means outstanding *invitations*: a self-registered account that never clicked its verification mail is nobody's to chase. + + One scan with ``COUNT(*) FILTER (WHERE …)`` per state rather than a + round-trip each (Postgres and SQLite >= 3.30 both support it). """ - counts: dict[str, int] = {} - for key, condition in ( - ("active", STATUS_CONDITIONS["active"]), - ("unverified", STATUS_CONDITIONS["unverified"]), - ("invited", STATUS_CONDITIONS["invited"]), - ): - stmt = select(func.count()).select_from(User).where(condition()) - counts[key] = int((await self._db.execute(stmt)).scalar_one()) - return counts + keys = ("active", "unverified", "invited") + stmt = select(*(func.count().filter(STATUS_CONDITIONS[k]()) for k in keys)).select_from( + User + ) + row = (await self._db.execute(stmt)).one() + return {key: int(value) for key, value in zip(keys, row, strict=True)} async def get_with_roles(self, user_id: uuid.UUID) -> User | None: return await self._get_user_with_roles(user_id) diff --git a/modules/users/users/auth_local/api.py b/modules/users/users/auth_local/api.py index 87609e67..0b9671d2 100644 --- a/modules/users/users/auth_local/api.py +++ b/modules/users/users/auth_local/api.py @@ -53,7 +53,7 @@ # ── Rate limit ─────────────────────────────────────────────────────────────── -def get_rate_limiter(request: Request) -> LoginRateLimiter: +async def get_rate_limiter(request: Request) -> LoginRateLimiter: """Return the per-app LoginRateLimiter built in UsersModule.on_startup.""" return request.app.state.users.rate_limiter diff --git a/modules/users/users/deps.py b/modules/users/users/deps.py index c021afbb..56499265 100644 --- a/modules/users/users/deps.py +++ b/modules/users/users/deps.py @@ -48,12 +48,12 @@ current_superuser = fastapi_users.current_user(active=True, superuser=True) -def get_mailer(request: Request): +async def get_mailer(request: Request): """Return the mailer from app.state.users (built in UsersModule.on_startup).""" return request.app.state.users.mailer -def get_event_bus(request: Request) -> EventBus: +async def get_event_bus(request: Request) -> EventBus: """Return the event bus from app.state.sm.""" return request.app.state.sm.event_bus