diff --git a/CHANGELOG.md b/CHANGELOG.md index b9da35de7..95f86a2fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,7 @@ All notable changes to this project are documented in this file. The format is b ### Changed +- **Breaking:** tenant member lists now use server pagination (`page` / `per_page`, default 25, maximum 200). The current-members API returns `{items, total, page, per_page, pages}` instead of an array; the Members page provides navigation and retains its page after mutations. `TenantService.list_members(tenant_id, params)` now takes `PageParams` and returns `Page[Membership]` instead of a list. - Cache env-supplied settings fields per class and default API version discovery per process; changes to environment, dotenv, or secret sources require a restart. - Pre-serialize shared Inertia i18n catalogs per locale, audience, and catalog load, preserving page JSON bytes and HTML escaping. - Reduce background task polling and list reads to their required columns; count task history separately from paginated reads and index newest-first, nulls-last ordering (migration `c5d8e2f1a903`, 2026-11-05 performance review). @@ -85,6 +86,7 @@ All notable changes to this project are documented in this file. The format is b ### Fixed +- Coalesce concurrent tenant membership and permission grant cache misses. In-flight invalidation retries against fresh authorization data; failures and cancellation release waiting readers. Cache unknown tenant subdomains for five seconds and drop the new slug's host lookups (on a new `tenants.hosts` invalidation channel, so no user id is read as a slug) and the owner's memberships after committed tenant creation; other users' caches are not cleared. Services built outside request dependencies pass `invalidate_hosts=make_host_invalidator(app)` beside `invalidate=make_invalidator(app)`. Retries after concurrent invalidation are bounded to three reads (2026-11-05 local performance review). - Offload S3 server-upload spool I/O, wait for in-flight I/O during cancellation, and reuse one client per multipart upload. Index the default tenant file browse order (migration `f6c2d4e8b109`; local performance review, 2026-11-05). - Users password creation, forgot-password fingerprints, reset verification and password updates offload hashing to worker threads, keeping unrelated async diff --git a/docs/framework/invalidation.md b/docs/framework/invalidation.md index fa9903c90..7c9a2aa0d 100644 --- a/docs/framework/invalidation.md +++ b/docs/framework/invalidation.md @@ -110,3 +110,41 @@ A standalone worker never builds the FastAPI app, so it installs no transport: i ## Still per-process `file_storage`'s aggregate cache remains per-process. Role overlays (`permissions.role_grants`), module settings (`settings.modules`, keyed by package) and feature flags (`feature_flags.overrides`, both scopes) subscribe through their modules' `register_invalidations` hooks. Their refresh tasks start and stop with module lifespan. + +## Concurrent cache fills + +`simple_module_core.singleflight.SingleFlight` serializes in-process fills for +one cache key. Keep one helper per app/cache and use `async with flight.hold(key)` +on a miss. Recheck the cache after acquiring the lock: an earlier reader may +already have filled it. Different keys proceed independently; idle keys are +removed after the final caller leaves. Failure or cancellation releases the +lock, so a waiter can retry without a cached failed task. + +The lock does not version or invalidate cached data. Authorization caches must +also compare their invalidation epoch before storing a DB read, and retry +against a fresh session if the epoch changed. `simple_module_core.singleflight.fill` +is that loop: + +```python +return await fill( + lambda: read_from_db(key), + epoch=lambda: _epoch, # bumped by every forget + store=lambda value: cache.__setitem__(key, value), + cached=lambda: cache.get(key, MISS), # MISS: nothing cached; None is a value +) +``` + +It reads at most `MAX_FILL_ATTEMPTS` (3) times. When every read raced an +invalidation it returns the last read **uncached** rather than loop while +invalidations keep arriving. That answer is not guaranteed to be newer than an +invalidation that landed during it: it is what an uncached read racing a +concurrent change would return, and it is never stored. Tenant memberships, +tenant subdomain lookups and direct permission grants use it inside their +per-key `SingleFlight` (subdomain lookups without one); their existing TTLs +still bound staleness from missed cross-worker invalidations. The users +session-version cache has not yet adopted the helper. + +Give each kind of key its own channel. A handler that dispatches on a key +prefix misreads any key of the other kind that happens to start with it (the +tenants module keeps user ids on `tenants.membership` and slugs on +`tenants.hosts` for this reason). diff --git a/docs/modules/tenants.md b/docs/modules/tenants.md index a055e7b76..87d81e723 100644 --- a/docs/modules/tenants.md +++ b/docs/modules/tenants.md @@ -141,6 +141,50 @@ Providers own persistence, period boundaries, idempotency and hold expiry. choose to raise it and map it to HTTP 402; the protocol's `consume` returns a decision rather than requiring an exception. +## Member pagination and resolver caches + +`GET /api/tenants/current/members` accepts `page` (1-based) and `per_page` +(default 25, maximum 200). It returns `{items, total, page, per_page, pages}`; +clients previously reading a bare array must read `items` and traverse the pages. +Invalid API parameters return 422. The Members page accepts the same parameters, +normalizes malformed values, and provides Previous/Next controls. Both order by +`(created_at, id)` and explicitly scope rows and totals to the active tenant. +Requests past the end clamp to the last page, including after a member removal; +mutations reload the current page and page size. Offset pagination reflects the +current roster, so concurrent additions/removals can shift page boundaries. + +Membership cache fills serialize concurrent misses per app/user, recheck the cache +when waiting ends, and retry if invalidated during a DB read. After three reads +that each raced an invalidation, the request answers with the last read without +caching it. That read is not guaranteed to be newer than an invalidation that +landed during it: it is equivalent to an uncached read racing a concurrent +change, and it is never cached. The 60-second membership TTL and invalidation +bus remain the fallback for changes in other workers. Permission grants and +subdomain lookups use the same `simple_module_core.singleflight.fill` loop; +grants keep their 30-second TTL. + +Unknown subdomain slugs are negatively cached for five seconds (bounded to 10,000 +entries); known slugs keep their 60-second TTL. Two invalidation channels carry +the evictions, both published after commit and delivered to every worker when a +transport is installed: + +| Channel | Key | Drops | +|---|---|---| +| `tenants.membership` (`INVALIDATION_CHANNEL`) | a user id | that user's memberships | +| | `None` | every membership **and** every host lookup | +| `tenants.hosts` (`HOSTS_INVALIDATION_CHANNEL`) | a slug | that slug's host lookups | +| | `None` | every host lookup | + +Membership keys are always user ids, whatever their shape (an OIDC subject can +be any string), so slugs never share their channel. Committed tenant creation +drops the owner's memberships and the new slug's host lookups; other users' +caches stay warm. Status changes and deletion publish `None` on the membership +channel. Construct services with both invalidators outside request +dependencies: `TenantService(db, invalidate=make_invalidator(app), +invalidate_hosts=make_host_invalidator(app))` (from `tenants.resolver` and +`tenants.host_resolver`). Direct SQL writes must publish `tenants.membership` +with `key=None` after commit or wait for TTL expiry. + ## Models `tenants_tenant` (id = the value stored in every `tenant_id`, slug, name, diff --git a/framework/core/simple_module_core/singleflight.py b/framework/core/simple_module_core/singleflight.py new file mode 100644 index 000000000..b9172043c --- /dev/null +++ b/framework/core/simple_module_core/singleflight.py @@ -0,0 +1,96 @@ +"""Serialize cache fills per key, and fill caches without storing stale reads. + +``SingleFlight`` serializes concurrent misses for one key; callers recheck their +cache inside the lock. Own one instance per application/cache. No background +task outlives its caller: failed or cancelled fills release the lock and the +next waiter retries normally. + +``fill`` is the epoch-checked read loop shared by caches that an invalidation +can drop mid-read: a read only gets stored if no invalidation landed while it +ran. +""" + +from __future__ import annotations + +import asyncio +from collections.abc import AsyncGenerator, Awaitable, Callable, Hashable +from contextlib import asynccontextmanager +from dataclasses import dataclass, field +from enum import Enum +from typing import Final, Literal + +# Epoch-checked fills retry at most this many reads; continuous invalidation +# must not keep a request looping. +MAX_FILL_ATTEMPTS = 3 + + +class Miss(Enum): + """Type of :data:`MISS`; a cache lookup's "nothing cached" answer.""" + + MISS = "miss" + + +MISS: Final = Miss.MISS + + +def _nothing_cached() -> Literal[Miss.MISS]: + return MISS + + +async def fill[T]( + read: Callable[[], Awaitable[T]], + *, + epoch: Callable[[], int], + store: Callable[[T], None], + cached: Callable[[], T | Literal[Miss.MISS]] = _nothing_cached, +) -> T: + """Read through *read* and *store* the result unless invalidated meanwhile. + + *epoch* returns the cache's invalidation counter (bumped by every forget); + a read whose epoch changed while it ran is not stored and is retried, after + rechecking *cached* (return :data:`MISS` when nothing is cached; ``None`` + is a valid cached value). At most :data:`MAX_FILL_ATTEMPTS` reads. + + After the last attempt the last read is returned uncached. It is **not** + guaranteed to be newer than an invalidation that landed during it: it is + equivalent to an uncached read racing a concurrent change, so the caller + answers this one request with it, and it is never cached. + """ + attempts = 0 + while True: + hit = cached() + if hit is not MISS: + return hit + started = epoch() + value = await read() + attempts += 1 + if epoch() == started: + store(value) + return value + if attempts >= MAX_FILL_ATTEMPTS: + return value + + +@dataclass +class _Flight: + lock: asyncio.Lock = field(default_factory=asyncio.Lock) + users: int = 0 + + +class SingleFlight: + def __init__(self) -> None: + self._locks: dict[Hashable, _Flight] = {} + + @asynccontextmanager + async def hold(self, key: Hashable) -> AsyncGenerator[None]: + flight = self._locks.get(key) + if flight is None: + flight = self._locks[key] = _Flight() + flight.users += 1 + try: + async with flight.lock: + yield + finally: + flight.users -= 1 + if not flight.users: + del self._locks[key] diff --git a/framework/core/tests/test_core_singleflight.py b/framework/core/tests/test_core_singleflight.py new file mode 100644 index 000000000..a6566e519 --- /dev/null +++ b/framework/core/tests/test_core_singleflight.py @@ -0,0 +1,100 @@ +"""Single-flight locks survive failures/cancellation and release idle keys.""" + +import asyncio + +import pytest +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS, MISS, SingleFlight, fill + + +@pytest.mark.parametrize("cancel_leader", [False, True]) +async def test_failure_or_cancellation_allows_next_reader(cancel_leader): + flight = SingleFlight() + started, release = asyncio.Event(), asyncio.Event() + + async def leader(): + async with flight.hold("key"): + started.set() + await release.wait() + raise ValueError("failed read") + + async def follower(): + async with flight.hold("key"): + return "fresh" + + first = asyncio.create_task(leader()) + await started.wait() + second = asyncio.create_task(follower()) + await asyncio.sleep(0) + if cancel_leader: + first.cancel() + else: + release.set() + with pytest.raises(asyncio.CancelledError if cancel_leader else ValueError): + await first + assert await second == "fresh" + assert not flight._locks + + +async def test_cancelled_waiter_does_not_unlock_leader_or_leak_key(): + flight = SingleFlight() + entered = asyncio.Event() + + async def waiter(): + async with flight.hold("key"): + entered.set() + + async with flight.hold("key"): + task = asyncio.create_task(waiter()) + await asyncio.sleep(0) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + assert not entered.is_set() + async with flight.hold("different"): + pass + assert not flight._locks + + +async def test_fill_stores_a_read_no_invalidation_raced(): + stored = [] + + async def read(): + return "value" + + assert await fill(read, epoch=lambda: 0, store=stored.append) == "value" + assert stored == ["value"] + + +async def test_fill_returns_a_cached_none_without_reading(): + async def read(): + raise AssertionError("must not read") + + assert await fill(read, epoch=lambda: 0, store=lambda _value: None, cached=lambda: None) is None + + +async def test_fill_retries_then_answers_uncached_under_continuous_invalidation(): + epoch, stored, reads = [0], [], [] + + async def read(): + reads.append(len(reads)) + epoch[0] += 1 # every read races an invalidation + return len(reads) + + result = await fill(read, epoch=lambda: epoch[0], store=stored.append, cached=lambda: MISS) + assert result == MAX_FILL_ATTEMPTS + assert len(reads) == MAX_FILL_ATTEMPTS + assert stored == [] + + +async def test_fill_rechecks_the_cache_before_retrying(): + epoch, cache = [0], {} + + async def read(): + epoch[0] += 1 + cache["k"] = "filled-by-another-reader" + return "raced" + + result = await fill( + read, epoch=lambda: epoch[0], store=lambda _value: None, cached=lambda: cache.get("k", MISS) + ) + assert result == "filled-by-another-reader" diff --git a/modules/api_tokens/tests/credentials/resolver/test_api_tokens_hardening.py b/modules/api_tokens/tests/credentials/resolver/test_api_tokens_hardening.py index 9508c849a..db4754211 100644 --- a/modules/api_tokens/tests/credentials/resolver/test_api_tokens_hardening.py +++ b/modules/api_tokens/tests/credentials/resolver/test_api_tokens_hardening.py @@ -32,7 +32,7 @@ async def _member_ids(resp: httpx.Response) -> set[str]: assert resp.status_code == 200, resp.text - return {str(m["user_id"]) for m in resp.json()} + return {str(m["user_id"]) for m in resp.json()["items"]} async def test_pat_is_pinned_to_its_tenant_whatever_the_header_says(app, actor, bearer): diff --git a/modules/permissions/permissions/grants.py b/modules/permissions/permissions/grants.py index f1f539b2c..9e56261f4 100644 --- a/modules/permissions/permissions/grants.py +++ b/modules/permissions/permissions/grants.py @@ -23,13 +23,15 @@ from cachetools import TTLCache from simple_module_core.app_state import get_services +from simple_module_core.invalidation import Invalidation +from simple_module_core.singleflight import MISS, SingleFlight, fill from sqlalchemy import select from permissions.models import UserPermission if TYPE_CHECKING: from fastapi import FastAPI, Request - from simple_module_core.invalidation import Invalidation, InvalidationBus + from simple_module_core.invalidation import InvalidationBus logger = logging.getLogger(__name__) @@ -54,6 +56,9 @@ """ _CACHES: WeakKeyDictionary = WeakKeyDictionary() +_FLIGHTS: WeakKeyDictionary[FastAPI, SingleFlight] = WeakKeyDictionary() +# Bumped by every invalidation so a read started earlier never caches stale grants. +_epoch = 0 def _cache(app: FastAPI) -> TTLCache: @@ -64,13 +69,9 @@ def _cache(app: FastAPI) -> TTLCache: return cache -_MISS = object() - - def clear_grants_cache() -> None: """Empty the cache — for tests that need a cold read.""" - for cache in list(_CACHES.values()): - cache.clear() + apply_invalidation(Invalidation(CHANNEL)) def _user_uuid(raw: Any) -> uuid.UUID | None: @@ -91,23 +92,34 @@ async def direct_grant_source(request: Request, user: Any) -> frozenset[str]: user_id = _user_uuid(getattr(user, "id", None)) if user_id is None: return frozenset() - # One ``get``, not ``in`` then ``[]``: a TTLCache entry can expire between. grants = _cache(request.app) - cached = grants.get(user_id, _MISS) - if cached is not _MISS: + # One ``get``, not ``in`` then ``[]``: a TTLCache entry can expire between. + cached = grants.get(user_id, MISS) + if cached is not MISS: return cached - session_factory = request.app.state.sm.db.session_factory - async with session_factory() as db: - result = await db.execute( - select(UserPermission.permission_key).where(UserPermission.user_id == user_id) + if (flight := _FLIGHTS.get(request.app)) is None: + flight = _FLIGHTS[request.app] = SingleFlight() + async with flight.hold(user_id): + + async def read() -> frozenset[str]: + async with request.app.state.sm.db.session_factory() as db: + result = await db.execute( + select(UserPermission.permission_key).where(UserPermission.user_id == user_id) + ) + return frozenset(result.scalars().all()) + + def store(keys: frozenset[str]) -> None: + grants[user_id] = keys + + return await fill( + read, epoch=lambda: _epoch, store=store, cached=lambda: grants.get(user_id, MISS) ) - keys = frozenset(result.scalars().all()) - grants[user_id] = keys - return keys def apply_invalidation(invalidation: Invalidation, app: FastAPI | None = None) -> None: """Drop grants for this app; app-less maintenance callers forget all caches.""" + global _epoch + _epoch += 1 if app is None: caches = list(_CACHES.values()) else: @@ -137,7 +149,6 @@ async def publish_grants_changed(app: FastAPI, user_id: uuid.UUID) -> None: bus = getattr(get_services(app), "invalidation", None) if bus is None: logger.debug("No invalidation bus on this app; dropping %s locally", user_id) - if (cache := _CACHES.get(app)) is not None: - cache.pop(user_id, None) + apply_invalidation(Invalidation(CHANNEL, key=str(user_id)), app) return await bus.publish(CHANNEL, key=str(user_id)) diff --git a/modules/permissions/tests/test_permissions_grants_singleflight.py b/modules/permissions/tests/test_permissions_grants_singleflight.py new file mode 100644 index 000000000..63596e3b4 --- /dev/null +++ b/modules/permissions/tests/test_permissions_grants_singleflight.py @@ -0,0 +1,86 @@ +"""Cold/expired grants coalesce; invalidated in-flight grants never authorize waiters.""" + +import asyncio +import uuid +from contextlib import asynccontextmanager +from types import SimpleNamespace + +import pytest +from cachetools import TTLCache +from fastapi import FastAPI +from permissions import grants +from simple_module_core.invalidation import Invalidation +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS +from starlette.requests import Request + + +@pytest.mark.parametrize("invalidate", [None, "user", "all", "clear", "publish"]) +@pytest.mark.parametrize("expired", [False, True]) +async def test_grants_coalesce_and_discard_invalidated_reads(invalidate, expired): + started, release = asyncio.Event(), asyncio.Event() + reads = 0 + user = SimpleNamespace(id=uuid.uuid4()) + + class DB: + async def execute(self, stmt): + nonlocal reads + reads += 1 + keys = ["secret.view"] if reads == 1 else [] + started.set() + await release.wait() + return SimpleNamespace(scalars=lambda: SimpleNamespace(all=lambda: keys)) + + @asynccontextmanager + async def session(): + yield DB() + + app = FastAPI() + app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + request = Request({"type": "http", "app": app}) + now = [0] + grants._CACHES[app] = TTLCache(maxsize=100, ttl=30, timer=lambda: now[0]) + if expired: + grants._CACHES[app][user.id] = frozenset({"expired"}) + now[0] = 31 + tasks = [asyncio.create_task(grants.direct_grant_source(request, user)) for _ in range(50)] + await started.wait() + await asyncio.sleep(0) + if invalidate == "clear": + grants.clear_grants_cache() + elif invalidate == "publish": + await grants.publish_grants_changed(app, user.id) + elif invalidate: + grants.apply_invalidation( + Invalidation(grants.CHANNEL, key=None if invalidate == "all" else str(user.id)), app + ) + release.set() + results = await asyncio.gather(*tasks) + assert results == [frozenset() if invalidate else frozenset({"secret.view"})] * 50 + assert reads == (2 if invalidate else 1) + assert not grants._FLIGHTS[app]._locks + + +async def test_grants_stop_retrying_under_continuous_invalidation(): + reads = 0 + user = SimpleNamespace(id=uuid.uuid4()) + + class DB: + async def execute(self, stmt): + nonlocal reads + reads += 1 + # Every read races an invalidation for some other user. + grants.apply_invalidation(Invalidation(grants.CHANNEL, key=str(uuid.uuid4()))) + keys = [f"read-{reads}"] + return SimpleNamespace(scalars=lambda: SimpleNamespace(all=lambda: keys)) + + @asynccontextmanager + async def session(): + yield DB() + + app = FastAPI() + app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + request = Request({"type": "http", "app": app}) + result = await grants.direct_grant_source(request, user) + assert reads == MAX_FILL_ATTEMPTS + assert result == frozenset({f"read-{MAX_FILL_ATTEMPTS}"}) + assert user.id not in grants._CACHES[app] diff --git a/modules/tenants/README.md b/modules/tenants/README.md index 6b75daf1a..e9db10352 100644 --- a/modules/tenants/README.md +++ b/modules/tenants/README.md @@ -44,6 +44,10 @@ scoped to them. | `POST /api/tenants/invitations/accept` | signed in as the invited email | Join | | `POST /api/tenants/admin/{id}/suspend` · `/reactivate` | `tenants.platform.manage` | Lifecycle | +Member lists accept `page` and `per_page` (default 25, max 200) and return +`{items, total, page, per_page, pages}`. The Members page uses the same pagination; +see [the tenant guide](../../docs/modules/tenants.md#member-pagination-and-resolver-caches). + Tenant-level routes act on the *active* tenant (`/current`), never on an id from the URL. diff --git a/modules/tenants/tenants/_membership_ops.py b/modules/tenants/tenants/_membership_ops.py index a5198f5e1..e3c338f5d 100644 --- a/modules/tenants/tenants/_membership_ops.py +++ b/modules/tenants/tenants/_membership_ops.py @@ -10,6 +10,7 @@ from typing import TYPE_CHECKING from simple_module_core.events import Event +from simple_module_db.pagination import Page, PageParams, paginate from sqlalchemy import func, select from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession @@ -32,6 +33,7 @@ def _after_commit( self, *events: Event, invalidate: Sequence[str | None] = (), + invalidate_hosts: Sequence[str | None] = (), callback: Callable[[], Awaitable[None]] | None = None, ) -> None: ... async def lock(self, tenant_id: str) -> Tenant: ... @@ -59,23 +61,28 @@ async def get_membership(self, tenant_id: str, user_id: str) -> Membership | Non ) return (await self.db.execute(stmt)).scalar_one_or_none() - async def list_members(self, tenant_id: str) -> list[Membership]: + async def list_members(self, tenant_id: str, params: PageParams) -> Page[Membership]: stmt = ( select(Membership) .where(col(Membership.tenant_id) == tenant_id) - .order_by(Membership.created_at) + .order_by(Membership.created_at, col(Membership.id)) ) - return list((await self.db.execute(stmt)).scalars().all()) + return await paginate(self.db, stmt, params) - async def seats_used(self, tenant_id: str) -> int: - """Members plus pending invitations — an invite reserves a seat.""" + async def seats_used(self, tenant_id: str, *, members: int | None = None) -> int: + """Members plus pending invitations — an invite reserves a seat. + + Pass *members* when the caller already counted every membership of the + tenant (``list_members(...).total``) to skip a second count. + """ from tenants.invitations import pending_invitation_count - members = await self.db.scalar( - select(func.count()) - .select_from(Membership) - .where(col(Membership.tenant_id) == tenant_id) - ) + if members is None: + members = await self.db.scalar( + select(func.count()) + .select_from(Membership) + .where(col(Membership.tenant_id) == tenant_id) + ) return int(members or 0) + await pending_invitation_count(self.db, tenant_id) async def ensure_seat_available(self, tenant_id: str) -> None: diff --git a/modules/tenants/tenants/constants.py b/modules/tenants/tenants/constants.py index c8b1e20f3..84758e9ef 100644 --- a/modules/tenants/tenants/constants.py +++ b/modules/tenants/tenants/constants.py @@ -20,7 +20,11 @@ # preference: the resolver re-validates it against a membership every request. SESSION_ACTIVE_TENANT = "sm_active_tenant" +# Key: a user id, or ``None`` for every user (which also drops every host lookup). INVALIDATION_CHANNEL = "tenants.membership" +# Key: a tenant slug, or ``None`` for every slug. Kept apart from the membership +# channel so no user id (an OIDC subject is any string) can be read as a slug. +HOSTS_INVALIDATION_CHANNEL = "tenants.hosts" # Entitlement keys this module enforces (see contracts.entitlements). ENTITLEMENT_SEATS = "tenants.seats" diff --git a/modules/tenants/tenants/contracts/schemas.py b/modules/tenants/tenants/contracts/schemas.py index 8e022668e..76aa9450b 100644 --- a/modules/tenants/tenants/contracts/schemas.py +++ b/modules/tenants/tenants/contracts/schemas.py @@ -8,6 +8,7 @@ from email_validator import EmailNotValidError, validate_email from pydantic import field_validator from simple_module_db import PLATFORM_TENANT_ID +from simple_module_db.pagination import Page from sqlmodel import Field, SQLModel from tenants.constants import MAX_EMAIL_LEN, MAX_NAME_LEN, MembershipRole @@ -60,6 +61,11 @@ class MemberView(SQLModel): joined_at: datetime | None = None +class MemberPage(Page[MemberView]): + # SQLModel needs a concrete field annotation to retain item types in OpenAPI. + items: list[MemberView] + + class RoleChange(SQLModel): role: MembershipRole diff --git a/modules/tenants/tenants/deps.py b/modules/tenants/tenants/deps.py index c66c23218..83b53ac1a 100644 --- a/modules/tenants/tenants/deps.py +++ b/modules/tenants/tenants/deps.py @@ -11,6 +11,7 @@ from tenants.constants import MANAGER_ROLES from tenants.errors import TenantError +from tenants.host_resolver import make_host_invalidator from tenants.invitations import InvitationService from tenants.resolver import make_invalidator from tenants.service import TenantService @@ -22,6 +23,7 @@ async def get_tenant_service(request: Request, db: AsyncSession = Depends(get_db db, bus=app.state.sm.event_bus, invalidate=make_invalidator(app), + invalidate_hosts=make_host_invalidator(app), invalidation_bus=app.state.sm.invalidation, entitlements=app.state.tenants.entitlements, ) diff --git a/modules/tenants/tenants/endpoints/api.py b/modules/tenants/tenants/endpoints/api.py index a2dded0cf..275d396a9 100644 --- a/modules/tenants/tenants/endpoints/api.py +++ b/modules/tenants/tenants/endpoints/api.py @@ -14,6 +14,7 @@ from fastapi import APIRouter, Depends, Request, Response from simple_module_core.permissions import grants from simple_module_core.user_directory import get_user_directory +from simple_module_hosting.pagination import PageParamsDep from simple_module_hosting.permissions import RequiresPermission, resolved_permissions_for from simple_module_hosting.token_guard import Authenticated @@ -28,6 +29,7 @@ InvitationCreate, InvitationIssued, InvitationView, + MemberPage, MemberView, MyTenantView, RoleChange, @@ -42,6 +44,7 @@ ) from tenants.errors import TenantError from tenants.invitation_mail import invitation_emailed +from tenants.models import Membership from tenants.resolver import switch_active router = APIRouter() @@ -107,12 +110,18 @@ async def switch_tenant( # ── active tenant: members ───────────────────────────────────── -@router.get("/current/members", response_model=list[MemberView], dependencies=_VIEW) -async def list_members(ctx: ActiveTenantDep, service: TenantServiceDep) -> list[MemberView]: - return [ - MemberView(user_id=m.user_id, email=m.email, role=m.role, joined_at=m.created_at) - for m in await service.list_members(ctx.tenant_id) - ] +def _member_view(m: Membership) -> MemberView: + return MemberView(user_id=m.user_id, email=m.email, role=m.role, joined_at=m.created_at) + + +@router.get("/current/members", response_model=MemberPage, dependencies=_VIEW) +async def list_members( + ctx: ActiveTenantDep, service: TenantServiceDep, params: PageParamsDep +) -> MemberPage: + page = await service.list_members(ctx.tenant_id, params) + return MemberPage( + **page.model_dump(exclude={"items"}), items=[_member_view(m) for m in page.items] + ) @router.patch("/current/members/{user_id}", response_model=MemberView, dependencies=_MANAGE) @@ -120,7 +129,7 @@ async def change_role( user_id: str, data: RoleChange, ctx: ManagerDep, service: TenantServiceDep ) -> MemberView: m = await service.change_role(ctx.tenant_id, user_id, data.role, actor_role=ctx.role) - return MemberView(user_id=m.user_id, email=m.email, role=m.role, joined_at=m.created_at) + return _member_view(m) @router.delete("/current/members/{user_id}", status_code=204, dependencies=_MANAGE) diff --git a/modules/tenants/tenants/endpoints/views.py b/modules/tenants/tenants/endpoints/views.py index 3b3908199..f85188acc 100644 --- a/modules/tenants/tenants/endpoints/views.py +++ b/modules/tenants/tenants/endpoints/views.py @@ -10,6 +10,7 @@ from fastapi.responses import RedirectResponse from simple_module_core.permissions import grants from simple_module_hosting.inertia_deps import InertiaDep +from simple_module_hosting.pagination import LenientPageParamsDep from simple_module_hosting.permissions import RequiresPermission, resolved_permissions_for from simple_module_inertia import InertiaResponse @@ -65,6 +66,7 @@ async def members( inertia: InertiaDep, service: TenantServiceDep, invitations: InvitationServiceDep, + params: LenientPageParamsDep, ) -> InertiaResponse | RedirectResponse: tenant_id = getattr(request.state, "tenant_id", None) tenant = await service.get(tenant_id) if tenant_id else None @@ -75,6 +77,9 @@ async def members( and getattr(request.state, "tenant_role", None) in MANAGER_ROLES ) pending = await invitations.list_pending(tenant.id) if can_manage else [] + page = await service.list_members(tenant.id, params) + # ``page.total`` counts the same rows as the seat count: every membership. + seats_used = await service.seats_used(tenant.id, members=page.total) limit = await service.entitlements.limit(tenant.id, ENTITLEMENT_SEATS) return await inertia.render( PAGE_MEMBERS, @@ -90,10 +95,11 @@ async def members( "role": m.role, "joined_at": m.created_at.isoformat() if m.created_at else None, } - for m in await service.list_members(tenant.id) + for m in page.items ], + "pagination": {"page": page.page, "per_page": page.per_page, "total": page.total}, "invitations": [i.model_dump(mode="json", exclude={"token_hash"}) for i in pending], - "seats": {"used": await service.seats_used(tenant.id), "limit": limit}, + "seats": {"used": seats_used, "limit": limit}, }, ) diff --git a/modules/tenants/tenants/host_resolver.py b/modules/tenants/tenants/host_resolver.py index 55f9667e9..a54404114 100644 --- a/modules/tenants/tenants/host_resolver.py +++ b/modules/tenants/tenants/host_resolver.py @@ -20,21 +20,68 @@ from __future__ import annotations +from typing import Literal + from cachetools import TTLCache from fastapi import FastAPI -from simple_module_core.app_state import get_public_routes +from simple_module_core.app_state import get_public_routes, get_services +from simple_module_core.invalidation import Invalidation, InvalidationBus +from simple_module_core.singleflight import MISS, Miss, fill from sqlalchemy import select from starlette.requests import Request -from tenants.constants import TenantStatus +from tenants.constants import HOSTS_INVALIDATION_CHANNEL, TenantStatus from tenants.models import Tenant HOST_TTL_SECONDS = 60 +NEGATIVE_HOST_TTL_SECONDS = 5 +_MISSING_SLUGS: TTLCache[str, bool] = TTLCache(maxsize=10_000, ttl=NEGATIVE_HOST_TTL_SECONDS) _BY_SLUG: TTLCache[str, tuple[str, str]] = TTLCache(maxsize=10_000, ttl=HOST_TTL_SECONDS) +# Bumped on every forget: a lookup that started earlier must not cache its stale row. +_epoch = 0 def forget_hosts() -> None: + global _epoch + _epoch += 1 _BY_SLUG.clear() + _MISSING_SLUGS.clear() + + +def forget_slug(slug: str) -> None: + global _epoch + _epoch += 1 + _BY_SLUG.pop(slug, None) + _MISSING_SLUGS.pop(slug, None) + + +def forget_host(slug: str | None) -> None: + """Drop one slug's lookups, or every slug's on ``None``.""" + if slug is None: + forget_hosts() + else: + forget_slug(slug) + + +def _apply_invalidation(inv: Invalidation) -> None: + forget_host(inv.key) + + +def subscribe_hosts(bus: InvalidationBus) -> None: + """Drop host lookups on ``HOSTS_INVALIDATION_CHANNEL`` (key: a slug, or ``None``).""" + bus.subscribe(HOSTS_INVALIDATION_CHANNEL, _apply_invalidation) + + +def make_host_invalidator(app: FastAPI): + """``TenantService(invalidate_hosts=...)``: forget here now, then tell the other workers.""" + + async def invalidate(slug: str | None) -> None: + forget_host(slug) + bus = getattr(get_services(app), "invalidation", None) + if bus is not None: + await bus.publish(HOSTS_INVALIDATION_CHANNEL, key=slug) + + return invalidate def subdomains_enabled(request: Request) -> bool: @@ -52,19 +99,31 @@ def subdomain_slug(request: Request) -> str | None: async def tenant_for_slug(app: FastAPI, slug: str) -> tuple[str, str] | None: - """``(tenant_id, status)`` for a slug, cached briefly.""" - hit = _BY_SLUG.get(slug) - if hit is not None: - return hit + """``(tenant_id, status)`` for a slug, cached briefly (unknown slugs too).""" + + def cached() -> tuple[str, str] | Literal[Miss.MISS] | None: + hit = _BY_SLUG.get(slug) + if hit is not None: + return hit + return None if _MISSING_SLUGS.get(slug) else MISS + + def store(row: tuple[str, str] | None) -> None: + if row is None: + _MISSING_SLUGS[slug] = True + else: + _BY_SLUG[slug] = row + + return await fill( + lambda: _read_slug(app, slug), epoch=lambda: _epoch, store=store, cached=cached + ) + + +async def _read_slug(app: FastAPI, slug: str) -> tuple[str, str] | None: async with app.state.sm.db.session_factory() as db: row = ( await db.execute(select(Tenant.id, Tenant.status).where(Tenant.slug == slug)) ).first() - if row is None: - return None - found = (row[0], row[1]) - _BY_SLUG[slug] = found - return found + return None if row is None else (row[0], row[1]) def is_public_route(request: Request) -> bool: @@ -83,9 +142,13 @@ async def resolve_from_host(request: Request, slug: str) -> tuple[str | None, bo __all__ = [ + "forget_host", "forget_hosts", + "forget_slug", "is_public_route", + "make_host_invalidator", "resolve_from_host", "subdomain_slug", + "subscribe_hosts", "tenant_for_slug", ] diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index a6bc580b5..1c303913a 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -119,9 +119,11 @@ def register_exception_handlers(self, app: FastAPI) -> None: install_exception_handlers(app) def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: + from tenants.host_resolver import subscribe_hosts from tenants.resolver import subscribe subscribe(bus) + subscribe_hosts(bus) def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from tenants.endpoints.admin import api as admin_api diff --git a/modules/tenants/tenants/pages/Members.tsx b/modules/tenants/tenants/pages/Members.tsx index 50238794e..b9891d554 100644 --- a/modules/tenants/tenants/pages/Members.tsx +++ b/modules/tenants/tenants/pages/Members.tsx @@ -1,6 +1,7 @@ import { Head, router } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; +import { PaginationBar } from '@simple-module-py/ui/components/PaginationBar'; import { Card } from '@simple-module-py/ui/components/ui/card'; import { AuthenticatedLayout } from '@simple-module-py/ui/layouts/AuthenticatedLayout'; import { type Invitation, InvitationsList } from '../components/InvitationsList'; @@ -18,6 +19,12 @@ interface Seats { limit: number | null; } +interface Pagination { + page: number; + per_page: number; + total: number; +} + interface Props { tenant: Tenant; my_role: MembershipRole; @@ -26,13 +33,32 @@ interface Props { members: Member[]; invitations: Invitation[]; seats: Seats; + pagination: Pagination; } -function Members({ tenant, my_role, my_user_id, can_manage, members, invitations, seats }: Props) { +function Members({ + tenant, + my_role, + my_user_id, + can_manage, + members, + invitations, + seats, + pagination, +}: Props) { const { t } = useT(); + const { page, per_page, total } = pagination; function reload() { - router.reload(); + visitPage(page); + } + + function visitPage(next: number) { + router.get( + '/tenants/members', + { page: next, per_page }, + { preserveState: true, preserveScroll: true }, + ); } const seatsLine = @@ -57,6 +83,7 @@ function Members({ tenant, my_role, my_user_id, can_manage, members, invitations tenantName={tenant.name} onChanged={reload} /> +
{seatsLine}
diff --git a/modules/tenants/tenants/pages/api.generated.ts b/modules/tenants/tenants/pages/api.generated.ts index 6241da596..e2ffe69d4 100644 --- a/modules/tenants/tenants/pages/api.generated.ts +++ b/modules/tenants/tenants/pages/api.generated.ts @@ -257,6 +257,19 @@ export interface components { /** Accepted At */ accepted_at?: string | null; }; + /** MemberPage */ + MemberPage: { + /** Items */ + items: components["schemas"]["MemberView"][]; + /** Total */ + total: number; + /** Page */ + page: number; + /** Per Page */ + per_page: number; + /** Pages */ + pages: number; + }; /** MemberView */ MemberView: { /** User Id */ @@ -515,7 +528,10 @@ export interface operations { }; tenants_list_members: { parameters: { - query?: never; + query?: { + page?: number; + per_page?: number; + }; header?: never; path?: never; cookie?: never; @@ -528,7 +544,16 @@ export interface operations { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["MemberView"][]; + "application/json": components["schemas"]["MemberPage"]; + }; + }; + /** @description Validation Error */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["HTTPValidationError"]; }; }; }; diff --git a/modules/tenants/tenants/resolver.py b/modules/tenants/tenants/resolver.py index c1016323d..06f9083b5 100644 --- a/modules/tenants/tenants/resolver.py +++ b/modules/tenants/tenants/resolver.py @@ -17,12 +17,14 @@ import dataclasses import logging from typing import Any +from weakref import WeakKeyDictionary from auth.contracts.tokens import AUTH_KIND_PAT, AUTH_KIND_SERVICE, AUTH_KIND_SESSION from cachetools import TTLCache from fastapi import FastAPI from simple_module_core.app_state import get_services from simple_module_core.invalidation import Invalidation, InvalidationBus +from simple_module_core.singleflight import MISS, SingleFlight, fill from simple_module_core.tenancy import is_tenant_role from simple_module_db import is_valid_tenant_id from simple_module_hosting.middleware import TenantResolution @@ -54,6 +56,9 @@ ) +_FLIGHTS: WeakKeyDictionary[FastAPI, SingleFlight] = WeakKeyDictionary() + + # Bumped by every invalidation. A read that started before an invalidation # must not store its (possibly stale) result afterwards: otherwise a member # removed while their own request was mid-read is re-cached as a member for a @@ -62,6 +67,11 @@ def forget(key: str | None) -> None: + """Drop one user's memberships, or every user's (and every host lookup) on ``None``. + + Keys on this channel are always user ids; slug evictions travel on + ``HOSTS_INVALIDATION_CHANNEL`` (see ``host_resolver``). + """ global _epoch _epoch += 1 if key is None: @@ -97,18 +107,30 @@ async def memberships_for(app: FastAPI, user_id: str) -> list[MyTenantView]: cached = _MEMBERSHIPS.get(user_id) if cached is not None: return cached - started = _epoch + if (flight := _FLIGHTS.get(app)) is None: + flight = _FLIGHTS[app] = SingleFlight() + async with flight.hold(user_id): + + def store(views: list[MyTenantView]) -> None: + _MEMBERSHIPS[user_id] = views + + return await fill( + lambda: _read_memberships(app, user_id), + epoch=lambda: _epoch, + store=store, + cached=lambda: _MEMBERSHIPS.get(user_id, MISS), + ) + + +async def _read_memberships(app: FastAPI, user_id: str) -> list[MyTenantView]: async with app.state.sm.db.session_factory() as db: rows = await TenantService(db).list_for_user(user_id) - views = [ + return [ MyTenantView( id=t.id, slug=t.slug, name=t.name, status=t.status, created_at=t.created_at, role=role ) for t, role in rows ] - if _epoch == started: - _MEMBERSHIPS[user_id] = views - return views def pick_active(memberships: list[MyTenantView], preferred: str | None) -> MyTenantView | None: diff --git a/modules/tenants/tenants/service.py b/modules/tenants/tenants/service.py index 5ffc4cf53..211703882 100644 --- a/modules/tenants/tenants/service.py +++ b/modules/tenants/tenants/service.py @@ -64,12 +64,14 @@ def __init__( *, bus: EventBus | None = None, invalidate: Invalidate = _no_invalidate, + invalidate_hosts: Invalidate = _no_invalidate, entitlements: EntitlementProvider | None = None, invalidation_bus: InvalidationBus | None = None, ) -> None: self.db = db self.bus = bus - self.invalidate = invalidate + self.invalidate = invalidate # membership channel: a user id, or None + self.invalidate_hosts = invalidate_hosts # hosts channel: a slug, or None self.invalidation_bus = invalidation_bus self.entitlements = entitlements or UnlimitedEntitlements() self.pending: list[Callable[[], Awaitable[Any]]] = [] @@ -78,11 +80,14 @@ def _after_commit( self, *events: Event, invalidate: Sequence[str | None] = (), + invalidate_hosts: Sequence[str | None] = (), callback: Callable[[], Awaitable[None]] | None = None, ) -> None: async def run() -> None: for key in invalidate: await self.invalidate(key) + for slug in invalidate_hosts: + await self.invalidate_hosts(slug) if self.bus is not None: for event in events: await self.bus.publish(event) @@ -137,6 +142,7 @@ async def create_tenant( TenantCreated(tenant.id, tenant.slug, tenant.name, owner_user_id), MembershipAdded(tenant.id, owner_user_id, MembershipRole.OWNER), invalidate=[owner_user_id], + invalidate_hosts=[tenant.slug], ) return tenant diff --git a/modules/tenants/tests-js/tenants_members_pagination.test.tsx b/modules/tenants/tests-js/tenants_members_pagination.test.tsx new file mode 100644 index 000000000..e547573b7 --- /dev/null +++ b/modules/tenants/tests-js/tenants_members_pagination.test.tsx @@ -0,0 +1,76 @@ +import '@testing-library/jest-dom/vitest'; +import { router } from '@inertiajs/react'; +import { configureI18n } from '@simple-module-py/i18n'; +import { cleanup, render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { afterEach, expect, test, vi } from 'vitest'; +import Members from '../tenants/pages/Members'; + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + usePage: () => ({ url: '/tenants/members' }), + router: { get: vi.fn() }, +})); +configureI18n({ + locale: 'en', + messages: { + 'ui.pagination.next': 'Next', + 'ui.pagination.previous': 'Previous', + 'ui.pagination.showing': 'Showing {from}–{to} of {total}', + 'tenants.members.remove_button': 'Remove', + 'tenants.members.remove_confirm_confirm': 'Confirm removal', + }, +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + vi.clearAllMocks(); + vi.unstubAllGlobals(); +}); + +const props = { + tenant: { id: 'org', slug: 'org', name: 'Org' }, + my_role: 'owner' as const, + my_user_id: 'owner', + can_manage: true, + members: [{ user_id: 'member', email: 'member@x.io', role: 'member' as const, joined_at: null }], + invitations: [], + seats: { used: 60, limit: null }, + pagination: { page: 2, per_page: 25, total: 60 }, +}; + +test('page navigation keeps server page size and uses server clamped page', async () => { + render(); + expect(screen.getByText('Showing 26–50 of 60')).toBeInTheDocument(); + await userEvent.click(screen.getByRole('button', { name: 'Next' })); + expect(router.get).toHaveBeenCalledWith( + '/tenants/members', + { page: 3, per_page: 25 }, + { preserveState: true, preserveScroll: true }, + ); + await userEvent.click(screen.getByRole('button', { name: 'Previous' })); + expect(router.get).toHaveBeenLastCalledWith( + '/tenants/members', + { page: 1, per_page: 25 }, + { preserveState: true, preserveScroll: true }, + ); +}); + +test('role mutation reloads the current page and page size', async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue( + new Response(JSON.stringify({}), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ), + ); + render(); + await userEvent.selectOptions(screen.getAllByRole('combobox')[0], 'admin'); + expect(router.get).toHaveBeenCalledWith( + '/tenants/members', + { page: 2, per_page: 25 }, + { preserveState: true, preserveScroll: true }, + ); +}); diff --git a/modules/tenants/tests/test_hardening.py b/modules/tenants/tests/test_hardening.py index f386112c3..7cb385a1b 100644 --- a/modules/tenants/tests/test_hardening.py +++ b/modules/tenants/tests/test_hardening.py @@ -112,7 +112,7 @@ async def slow(self, user_id): resolver.forget("u-1") # the membership changed while the read was in flight release.set() await read - assert resolver.cached_memberships("u-1") is None + assert resolver.cached_memberships("u-1") == [] async def test_slug_shape_is_enforced(user_client): diff --git a/modules/tenants/tests/test_invitations.py b/modules/tenants/tests/test_invitations.py index 753ee3d42..617d5df91 100644 --- a/modules/tenants/tests/test_invitations.py +++ b/modules/tenants/tests/test_invitations.py @@ -35,7 +35,7 @@ async def test_invite_and_accept(user_client): assert accepted.json()["id"] == tenant["id"] assert accepted.json()["role"] == "admin" - members = (await owner.get("/api/tenants/current/members")).json() + members = (await owner.get("/api/tenants/current/members")).json()["items"] assert {m["user_id"] for m in members} >= {new_id} again = await new.post("/api/tenants/invitations/accept", json={"token": token}) diff --git a/modules/tenants/tests/test_membership_changes.py b/modules/tenants/tests/test_membership_changes.py index e17729afc..724bb35dc 100644 --- a/modules/tenants/tests/test_membership_changes.py +++ b/modules/tenants/tests/test_membership_changes.py @@ -60,5 +60,5 @@ async def test_joining_and_switching_tenants_needs_no_relogin(tenant_client): assert mine == {first, second} assert (await user.client.post(f"/api/tenants/{first}/switch")).status_code == 204 - members = (await user.client.get("/api/tenants/current/members")).json() + members = (await user.client.get("/api/tenants/current/members")).json()["items"] assert [m["user_id"] for m in members] == [user.user_id] diff --git a/modules/tenants/tests/test_subdomains.py b/modules/tenants/tests/test_subdomains.py index 0eb870b46..9c764feaa 100644 --- a/modules/tenants/tests/test_subdomains.py +++ b/modules/tenants/tests/test_subdomains.py @@ -92,4 +92,4 @@ async def test_member_on_their_subdomain_gets_their_role(site, user_client): owner.base_url = httpx.URL("http://acme.example.com") assert (await owner.get("/api/private/pages")).json() == ["acme-home"] members = await owner.get("/api/tenants/current/members") - assert members.status_code == 200 and members.json()[0]["role"] == "owner" + assert members.status_code == 200 and members.json()["items"][0]["role"] == "owner" diff --git a/modules/tenants/tests/test_tenant_client_fixture.py b/modules/tenants/tests/test_tenant_client_fixture.py index 3d3f5b6ef..1d30fed15 100644 --- a/modules/tenants/tests/test_tenant_client_fixture.py +++ b/modules/tenants/tests/test_tenant_client_fixture.py @@ -9,7 +9,7 @@ @pytest.mark.parametrize("role", list(TenantRole)) async def test_fixture_creates_a_member_with_the_role(tenant_client, role): async with tenant_client(role) as (client, tenant_id, user_id): - members = (await client.get("/api/tenants/current/members")).json() + members = (await client.get("/api/tenants/current/members")).json()["items"] assert [(m["user_id"], m["role"]) for m in members] == [(user_id, role)] mine = (await client.get("/api/tenants/")).json() assert [t["id"] for t in mine] == [tenant_id] diff --git a/modules/tenants/tests/test_tenants_api.py b/modules/tenants/tests/test_tenants_api.py index 1e48d4bbd..070ee70d3 100644 --- a/modules/tenants/tests/test_tenants_api.py +++ b/modules/tenants/tests/test_tenants_api.py @@ -15,7 +15,7 @@ async def test_creator_becomes_owner_and_active(user_client): assert tenant["role"] == "owner" assert tenant["slug"] == "acme-inc" - members = (await client.get("/api/tenants/current/members")).json() + members = (await client.get("/api/tenants/current/members")).json()["items"] assert [(m["user_id"], m["role"], m["email"]) for m in members] == [ (user_id, "owner", "owner@acme.io") ] diff --git a/modules/tenants/tests/test_tenants_cache_singleflight.py b/modules/tenants/tests/test_tenants_cache_singleflight.py new file mode 100644 index 000000000..853134fcc --- /dev/null +++ b/modules/tenants/tests/test_tenants_cache_singleflight.py @@ -0,0 +1,90 @@ +"""Concurrent misses must share successful reads without sharing stale authorization.""" + +import asyncio +from contextlib import asynccontextmanager +from types import SimpleNamespace + +import pytest +from cachetools import TTLCache +from fastapi import FastAPI +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS +from tenants import resolver +from tenants.service import TenantService + + +@pytest.mark.parametrize("invalidate", [None, "same", "all"]) +@pytest.mark.parametrize("expired", [False, True]) +async def test_memberships_coalesce_and_retry_invalidation(monkeypatch, invalidate, expired): + app = FastAPI() + now = [0] + monkeypatch.setattr( + resolver, "_MEMBERSHIPS", TTLCache(maxsize=100, ttl=60, timer=lambda: now[0]) + ) + if expired: + resolver._MEMBERSHIPS["same"] = [] + now[0] = 61 + + @asynccontextmanager + async def session(): + yield None + + app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + started, release = asyncio.Event(), asyncio.Event() + reads = 0 + + async def read(self, user_id): + nonlocal reads + reads += 1 + started.set() + rows = ( + [ + ( + SimpleNamespace( + id="org", slug="org", name="Org", status="active", created_at=None + ), + "owner", + ) + ] + if reads == 1 + else [] + ) + await release.wait() + return rows + + monkeypatch.setattr(TenantService, "list_for_user", read) + tasks = [asyncio.create_task(resolver.memberships_for(app, "same")) for _ in range(50)] + await started.wait() + await asyncio.sleep(0) + if invalidate: + resolver.forget(None if invalidate == "all" else "same") + release.set() + results = await asyncio.gather(*tasks) + assert [[m.role for m in result] for result in results] == ( + [[]] if invalidate else [["owner"]] + ) * 50 + assert not resolver._FLIGHTS[app]._locks + assert reads == (2 if invalidate else 1) + + +async def test_memberships_stop_retrying_under_continuous_invalidation(monkeypatch): + app = FastAPI() + + @asynccontextmanager + async def session(): + yield None + + app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + reads = 0 + + async def read(self, user_id): + nonlocal reads + reads += 1 + resolver.forget("someone-else") # every read races an invalidation + org = SimpleNamespace(id="org", slug="org", name="Org", status="active", created_at=None) + return [(org, f"read-{reads}")] + + monkeypatch.setattr(TenantService, "list_for_user", read) + result = await resolver.memberships_for(app, "same") + assert reads == MAX_FILL_ATTEMPTS + assert [m.role for m in result] == [f"read-{MAX_FILL_ATTEMPTS}"] + assert resolver.cached_memberships("same") is None diff --git a/modules/tenants/tests/test_tenants_invitation_verification.py b/modules/tenants/tests/test_tenants_invitation_verification.py index 8c1ee175e..6a0d5cff3 100644 --- a/modules/tenants/tests/test_tenants_invitation_verification.py +++ b/modules/tenants/tests/test_tenants_invitation_verification.py @@ -29,7 +29,7 @@ async def test_unverified_account_cannot_accept_invitation(app, user_client): rejected = await bob.post("/api/tenants/invitations/accept", json={"token": token}) assert rejected.status_code == 403 assert rejected.json()["detail"] == "invitation_email_unverified" - members = (await owner.get("/api/tenants/current/members")).json() + members = (await owner.get("/api/tenants/current/members")).json()["items"] assert bob_id not in {member["user_id"] for member in members} pending = (await owner.get("/api/tenants/current/invitations")).json() assert issued.json()["id"] in {invitation["id"] for invitation in pending} diff --git a/modules/tenants/tests/test_tenants_members_pagination.py b/modules/tenants/tests/test_tenants_members_pagination.py new file mode 100644 index 000000000..3da58e6a6 --- /dev/null +++ b/modules/tenants/tests/test_tenants_members_pagination.py @@ -0,0 +1,92 @@ +"""Bounded, tenant-scoped member pages with stable ordering and complete traversal.""" + +from datetime import UTC, datetime + +import pytest +from fastapi import FastAPI +from sqlalchemy import delete, select +from tenants.endpoints.api import router +from tenants.models import Membership, Tenant + + +async def test_members_api_and_page_paginate_and_clamp(app, tenant_client): + async with tenant_client("owner") as (client, tenant_id, owner_id): + async with app.state.sm.db.session_factory() as db: + owner = await db.scalar( + select(Membership).where( + Membership.tenant_id == tenant_id, Membership.user_id == owner_id + ) + ) + # Use a tied timestamp so the primary-key tie breaker matters. + joined = datetime(2026, 1, 1, tzinfo=UTC) + assert owner is not None + owner.created_at = joined + foreign = Tenant(name="Foreign", slug="foreign") + db.add(foreign) + await db.flush() + for i in range(59): + db.add(Membership(tenant_id=tenant_id, user_id=f"member-{i}", created_at=joined)) + db.add(Membership(tenant_id=foreign.id, user_id="outsider")) + await db.commit() + + pages = [] + for page in (1, 2, 3): + response = await client.get(f"/api/tenants/current/members?page={page}") + assert response.status_code == 200 + data = response.json() + assert (data["total"], data["page"], data["per_page"], data["pages"]) == ( + 60, + page, + 25, + 3, + ) + pages.extend(m["user_id"] for m in data["items"]) + assert pages == [owner_id, *[f"member-{i}" for i in range(59)]] + props = ( + await client.get("/tenants/members?page=99&per_page=10", headers={"X-Inertia": "true"}) + ).json()["props"] + assert props["pagination"] == {"page": 6, "per_page": 10, "total": 60} + assert len(props["members"]) == 10 + # Removing the final page's last member must return the previous valid page. + last = (await client.get("/api/tenants/current/members?page=60&per_page=1")).json()[ + "items" + ][0] + assert ( + await client.delete(f"/api/tenants/current/members/{last['user_id']}") + ).status_code == 204 + data = (await client.get("/api/tenants/current/members?page=60&per_page=1")).json() + assert (data["page"], data["total"]) == (59, 59) + + +@pytest.mark.parametrize("query", ["page=0", "page=oops", "per_page=0", "per_page=201"]) +async def test_members_api_rejects_invalid_pagination(tenant_client, query): + async with tenant_client("owner") as (client, _, _): + assert (await client.get(f"/api/tenants/current/members?{query}")).status_code == 422 + + +async def test_empty_roster_and_lenient_page_parameters(app, tenant_client): + async with tenant_client("owner") as (client, tenant_id, _): + props = ( + await client.get( + "/tenants/members?page=oops&per_page=-1", headers={"X-Inertia": "true"} + ) + ).json()["props"] + assert props["pagination"] == {"page": 1, "per_page": 1, "total": 1} + # Keep the already-authorized request's membership cache warm to exercise + # an empty roster independently of the tenant access gate. + async with app.state.sm.db.session_factory() as db: + await db.execute(delete(Membership).where(Membership.tenant_id == tenant_id)) + await db.commit() + data = (await client.get("/api/tenants/current/members?page=999")).json() + assert data == {"items": [], "total": 0, "page": 1, "per_page": 25, "pages": 1} + + +def test_members_openapi_preserves_typed_items(): + application = FastAPI() + application.include_router(router) + schema = application.openapi() + response = schema["paths"]["/current/members"]["get"]["responses"]["200"]["content"][ + "application/json" + ]["schema"] + model = schema["components"]["schemas"][response["$ref"].split("/")[-1]] + assert model["properties"]["items"]["items"] == {"$ref": "#/components/schemas/MemberView"} diff --git a/modules/tenants/tests/test_tenants_negative_host_cache.py b/modules/tenants/tests/test_tenants_negative_host_cache.py new file mode 100644 index 000000000..e33820c3b --- /dev/null +++ b/modules/tenants/tests/test_tenants_negative_host_cache.py @@ -0,0 +1,157 @@ +"""Unknown hosts expire quickly and committed tenant creation invalidates only its slug.""" + +import asyncio +from contextlib import asynccontextmanager +from types import SimpleNamespace + +from cachetools import TTLCache +from fastapi import FastAPI +from simple_module_core.invalidation import Invalidation +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS +from simple_module_db.transaction import finalize_session +from tenants import host_resolver, resolver +from tenants.constants import HOSTS_INVALIDATION_CHANNEL, INVALIDATION_CHANNEL +from tenants.contracts.schemas import TenantCreate +from tenants.service import TenantService + + +def _fake_app(db) -> FastAPI: + @asynccontextmanager + async def session(): + yield db + + app = FastAPI() + app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + return app + + +async def test_unknown_slug_is_cached_until_expiry_or_invalidation(monkeypatch): + now = [0] + monkeypatch.setattr( + host_resolver, "_MISSING_SLUGS", TTLCache(maxsize=10_000, ttl=5, timer=lambda: now[0]) + ) + reads = 0 + + class DB: + async def execute(self, stmt): + nonlocal reads + reads += 1 + return SimpleNamespace(first=lambda: None) + + app = _fake_app(DB()) + for _ in range(50): + assert await host_resolver.tenant_for_slug(app, "missing") is None + assert reads == 1 + now[0] = 6 + await host_resolver.tenant_for_slug(app, "missing") + assert reads == 2 + host_resolver.forget_hosts() + await host_resolver.tenant_for_slug(app, "missing") + assert reads == 3 + + +async def test_previously_missing_slug_resolves_right_after_create(app, user_client): + assert await host_resolver.tenant_for_slug(app, "new-org") is None + async with user_client("new-owner@x.io") as (client, _): + response = await client.post("/api/tenants/", json={"name": "New", "slug": "new-org"}) + assert response.status_code == 201 + hit = await host_resolver.tenant_for_slug(app, "new-org") + assert hit is not None and hit[0] == response.json()["id"] + + +async def test_create_drops_the_missing_slug_only_after_commit(app, user_client): + async with user_client("owner@x.io") as (_, owner_id): + async with app.state.sm.db.session_factory() as db: + service = TenantService( + db, + invalidate=resolver.make_invalidator(app), + invalidate_hosts=host_resolver.make_host_invalidator(app), + ) + assert await host_resolver.tenant_for_slug(app, "fresh") is None + tenant = await service.create_tenant( + TenantCreate(name="Fresh", slug="fresh"), owner_user_id=owner_id + ) + assert await host_resolver.tenant_for_slug(app, "fresh") is None + await finalize_session(db) + hit = await host_resolver.tenant_for_slug(app, "fresh") + assert hit is not None and hit[0] == tenant.id + + +async def test_create_keeps_other_users_memberships_and_slugs(app, tenant_client, user_client): + async with tenant_client("owner") as (_, tenant_id, bystander_id): + bystander = await resolver.memberships_for(app, bystander_id) + existing = bystander[0].slug + cached_host = await host_resolver.tenant_for_slug(app, existing) + assert cached_host is not None and cached_host[0] == tenant_id + async with user_client("creator@x.io") as (client, _): + response = await client.post("/api/tenants/", json={"name": "Other", "slug": "other"}) + assert response.status_code == 201 + assert resolver.cached_memberships(bystander_id) is bystander + assert host_resolver._BY_SLUG.get(existing) == cached_host + + +async def test_host_key_from_the_bus_drops_one_slug_and_no_memberships(app): + resolver._MEMBERSHIPS["someone"] = [] + host_resolver._BY_SLUG["kept"] = ("kept-id", "active") + host_resolver._MISSING_SLUGS["created"] = True + await app.state.sm.invalidation.publish(HOSTS_INVALIDATION_CHANNEL, key="created") + assert "created" not in host_resolver._MISSING_SLUGS + assert host_resolver._BY_SLUG.get("kept") == ("kept-id", "active") + assert resolver.cached_memberships("someone") == [] + await app.state.sm.invalidation.publish(HOSTS_INVALIDATION_CHANNEL, key=None) + assert "kept" not in host_resolver._BY_SLUG + assert resolver.cached_memberships("someone") == [] + + +def test_membership_key_that_looks_like_a_host_key_is_a_user_id(): + """An OIDC subject is any string; ``host:x`` on the membership channel is a user.""" + subject = "host:kept" + resolver._MEMBERSHIPS[subject] = [] + host_resolver._BY_SLUG["kept"] = ("kept-id", "active") + resolver._apply_invalidation(Invalidation(INVALIDATION_CHANNEL, key=subject)) + assert resolver.cached_memberships(subject) is None + assert host_resolver._BY_SLUG.get("kept") == ("kept-id", "active") + + +async def test_host_invalidation_during_read_retries_before_caching(): + started, release = asyncio.Event(), asyncio.Event() + reads = 0 + + class DB: + async def execute(self, stmt): + nonlocal reads + reads += 1 + row = None if reads == 1 else ("new-tenant", "active") + started.set() + await release.wait() + return SimpleNamespace(first=lambda: row) + + app = _fake_app(DB()) + task = asyncio.create_task(host_resolver.tenant_for_slug(app, "racing")) + await started.wait() + host_resolver.forget_slug("racing") + release.set() + assert await task == ("new-tenant", "active") + assert await host_resolver.tenant_for_slug(app, "racing") == ("new-tenant", "active") + assert reads == 2 + + +async def test_host_lookup_stops_retrying_under_continuous_invalidation(): + reads = 0 + + class DB: + async def execute(self, stmt): + nonlocal reads + reads += 1 + host_resolver.forget_slug("elsewhere") # every read races an invalidation + row = (f"read-{reads}", "active") + return SimpleNamespace(first=lambda: row) + + app = _fake_app(DB()) + assert await host_resolver.tenant_for_slug(app, "busy") == ( + f"read-{MAX_FILL_ATTEMPTS}", + "active", + ) + assert reads == MAX_FILL_ATTEMPTS + assert "busy" not in host_resolver._BY_SLUG + assert "busy" not in host_resolver._MISSING_SLUGS