From 7a79692711edbdfa9bf065e6a56ed0474a312740 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 18:05:54 +0100 Subject: [PATCH 1/5] perf(tenants,permissions): paginate members, cache unknown subdomains, single-flight auth cache fills Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 + docs/framework/invalidation.md | 16 +++ docs/modules/tenants.md | 27 ++++ .../core/simple_module_core/singleflight.py | 35 +++++ .../core/tests/test_core_singleflight.py | 55 ++++++++ modules/permissions/permissions/grants.py | 37 +++-- .../test_permissions_grants_singleflight.py | 59 ++++++++ modules/tenants/README.md | 4 + modules/tenants/tenants/_membership_ops.py | 7 +- modules/tenants/tenants/contracts/schemas.py | 6 + modules/tenants/tenants/endpoints/api.py | 20 ++- modules/tenants/tenants/endpoints/views.py | 8 +- modules/tenants/tenants/host_resolver.py | 37 +++-- modules/tenants/tenants/pages/Members.tsx | 28 +++- .../tenants/tenants/pages/api.generated.ts | 29 +++- modules/tenants/tenants/resolver.py | 40 ++++-- modules/tenants/tenants/service.py | 15 ++- .../tenants_members_pagination.test.tsx | 79 +++++++++++ modules/tenants/tests/test_hardening.py | 2 +- modules/tenants/tests/test_invitations.py | 2 +- .../tenants/tests/test_membership_changes.py | 2 +- modules/tenants/tests/test_subdomains.py | 2 +- .../tests/test_tenant_client_fixture.py | 2 +- modules/tenants/tests/test_tenants_api.py | 2 +- .../tests/test_tenants_cache_singleflight.py | 65 +++++++++ .../test_tenants_invitation_verification.py | 2 +- .../tests/test_tenants_members_pagination.py | 95 +++++++++++++ .../tests/test_tenants_negative_host_cache.py | 127 ++++++++++++++++++ 28 files changed, 747 insertions(+), 58 deletions(-) create mode 100644 framework/core/simple_module_core/singleflight.py create mode 100644 framework/core/tests/test_core_singleflight.py create mode 100644 modules/permissions/tests/test_permissions_grants_singleflight.py create mode 100644 modules/tenants/tests-js/tenants_members_pagination.test.tsx create mode 100644 modules/tenants/tests/test_tenants_cache_singleflight.py create mode 100644 modules/tenants/tests/test_tenants_members_pagination.py create mode 100644 modules/tenants/tests/test_tenants_negative_host_cache.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 342e29dac..7538d6dbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,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. - Scaffold new modules with `core/` and `web/` areas and mirrored nested Python tests; existing modules migrate individually. See `docs/module-layout.md`. - `mail` follows the module layout: `mail.core.{constants,settings,state,data_compatibility}` and `mail.delivery.{sender,runtime}`. `mail.settings` and `mail.constants` still import, with a `DeprecationWarning`, until 0.0.46. - `site_lock` follows the module layout: `site_lock.core.{constants,settings,state}` and `site_lock.web.{middleware,page,rate_limit}` (the unlock template moves with the page). `site_lock.settings` still imports, with a `DeprecationWarning`, until 0.0.46. @@ -24,6 +25,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 invalidate host lookups after committed tenant creation or renaming (2026-11-05 local performance review). - Include module `ui/` sources in wheel-installed Tailwind scanning and the asset manifest; retain legacy `components/` support. ## [0.0.43] — 2026-11-04 diff --git a/docs/framework/invalidation.md b/docs/framework/invalidation.md index fa9903c90..517a50cf0 100644 --- a/docs/framework/invalidation.md +++ b/docs/framework/invalidation.md @@ -110,3 +110,19 @@ 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 helper does not version or invalidate cached data. Authorization caches must +also compare their invalidation epoch before storing **or returning** a DB read, +and retry against a fresh session if the epoch changed. Tenant memberships and +direct permission grants do this; their existing TTLs still bound staleness from +missed cross-worker invalidations. The users session-version cache has not yet +adopted the helper. diff --git a/docs/modules/tenants.md b/docs/modules/tenants.md index a055e7b76..0460040f0 100644 --- a/docs/modules/tenants.md +++ b/docs/modules/tenants.md @@ -141,6 +141,33 @@ 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. No stale read is +returned to avoid waiting. The 60-second membership TTL and invalidation bus +remain the fallback for changes in other workers. Permission grants use the same +shared single-flight helper with their existing 30-second TTL. + +Unknown subdomain slugs are negatively cached for five seconds (bounded to 10,000 +entries); known slugs keep their 60-second TTL. Committed tenant creation, +`TenantService.rename_tenant(tenant_id, TenantCreate(...))`, status changes and +deletion invalidate host lookups via the tenant invalidator. Renaming also clears +cached membership names/slugs. Construct services with the normal invalidator +(`make_invalidator(app)` outside request dependencies); direct SQL writes must +publish the tenant invalidation channel with `key=None` after commit or wait for +TTL expiry. No new rename endpoint is exposed. + ## 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..591e395a4 --- /dev/null +++ b/framework/core/simple_module_core/singleflight.py @@ -0,0 +1,35 @@ +"""Serialize cache fills per 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. +""" + +from __future__ import annotations + +import asyncio +from collections.abc import AsyncGenerator, Hashable +from contextlib import asynccontextmanager +from dataclasses import dataclass, field + + +@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.setdefault(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..05e47990a --- /dev/null +++ b/framework/core/tests/test_core_singleflight.py @@ -0,0 +1,55 @@ +"""Single-flight locks survive failures/cancellation and release idle keys.""" + +import asyncio + +import pytest +from simple_module_core.singleflight import SingleFlight + + +@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 diff --git a/modules/permissions/permissions/grants.py b/modules/permissions/permissions/grants.py index f1f539b2c..9cf7ffab6 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 SingleFlight 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,8 @@ """ _CACHES: WeakKeyDictionary = WeakKeyDictionary() +_FLIGHTS: WeakKeyDictionary = WeakKeyDictionary() +_epoch = 0 def _cache(app: FastAPI) -> TTLCache: @@ -69,6 +73,8 @@ def _cache(app: FastAPI) -> TTLCache: def clear_grants_cache() -> None: """Empty the cache — for tests that need a cold read.""" + global _epoch + _epoch += 1 for cache in list(_CACHES.values()): cache.clear() @@ -91,23 +97,31 @@ 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: 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) - ) - keys = frozenset(result.scalars().all()) - grants[user_id] = keys - return keys + flight = _FLIGHTS.setdefault(request.app, SingleFlight()) + async with flight.hold(user_id): + while True: + cached = grants.get(user_id, _MISS) + if cached is not _MISS: + return cached + started = _epoch + 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) + ) + keys = frozenset(result.scalars().all()) + if started == _epoch: + 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 +151,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..421f9ad30 --- /dev/null +++ b/modules/permissions/tests/test_permissions_grants_singleflight.py @@ -0,0 +1,59 @@ +"""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 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 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..3c1b61023 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 @@ -59,13 +60,13 @@ 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.""" 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/endpoints/api.py b/modules/tenants/tenants/endpoints/api.py index a2dded0cf..ce1ddaf1e 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, @@ -107,12 +109,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) - ] +@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=[ + MemberView(user_id=m.user_id, email=m.email, role=m.role, joined_at=m.created_at) + for m in page.items + ], + ) @router.patch("/current/members/{user_id}", response_model=MemberView, dependencies=_MANAGE) diff --git a/modules/tenants/tenants/endpoints/views.py b/modules/tenants/tenants/endpoints/views.py index 3b3908199..fc3bee188 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,7 @@ 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) limit = await service.entitlements.limit(tenant.id, ENTITLEMENT_SEATS) return await inertia.render( PAGE_MEMBERS, @@ -90,8 +93,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 ], + "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}, }, diff --git a/modules/tenants/tenants/host_resolver.py b/modules/tenants/tenants/host_resolver.py index 55f9667e9..32285da99 100644 --- a/modules/tenants/tenants/host_resolver.py +++ b/modules/tenants/tenants/host_resolver.py @@ -30,11 +30,17 @@ 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) +_epoch = 0 _BY_SLUG: TTLCache[str, tuple[str, str]] = TTLCache(maxsize=10_000, ttl=HOST_TTL_SECONDS) def forget_hosts() -> None: + global _epoch + _epoch += 1 _BY_SLUG.clear() + _MISSING_SLUGS.clear() def subdomains_enabled(request: Request) -> bool: @@ -53,18 +59,25 @@ 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 - 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 + while True: + hit = _BY_SLUG.get(slug) + if hit is not None: + return hit + if _MISSING_SLUGS.get(slug): + return None + started = _epoch + 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 started != _epoch: + continue + if row is None: + _MISSING_SLUGS[slug] = True + return None + found = (row[0], row[1]) + _BY_SLUG[slug] = found + return found def is_public_route(request: Request) -> bool: diff --git a/modules/tenants/tenants/pages/Members.tsx b/modules/tenants/tenants/pages/Members.tsx index 50238794e..5c0c07975 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'; @@ -26,13 +27,35 @@ interface Props { members: Member[]; invitations: Invitation[]; seats: Seats; + page: number; + per_page: number; + total: number; } -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, + page, + per_page, + total, +}: Props) { const { t } = useT(); 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 +80,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..47042d2ee 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 SingleFlight 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 @@ -97,18 +102,29 @@ 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 - async with app.state.sm.db.session_factory() as db: - rows = await TenantService(db).list_for_user(user_id) - views = [ - 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 + flight = _FLIGHTS.setdefault(app, SingleFlight()) + async with flight.hold(user_id): + while True: + cached = _MEMBERSHIPS.get(user_id) + if cached is not None: + return cached + started = _epoch + async with app.state.sm.db.session_factory() as db: + rows = await TenantService(db).list_for_user(user_id) + views = [ + 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..71cd0605b 100644 --- a/modules/tenants/tenants/service.py +++ b/modules/tenants/tenants/service.py @@ -136,10 +136,23 @@ async def create_tenant( self._after_commit( TenantCreated(tenant.id, tenant.slug, tenant.name, owner_user_id), MembershipAdded(tenant.id, owner_user_id, MembershipRole.OWNER), - invalidate=[owner_user_id], + invalidate=[None], ) return tenant + async def rename_tenant(self, tenant_id: str, data: TenantCreate) -> Tenant: + """Rename an organisation; slug changes drop host and membership caches on commit.""" + tenant = await self.lock(tenant_id) + tenant.name = data.name + if data.slug is not None: + tenant.slug = data.slug + try: + await self.db.flush() + except IntegrityError as exc: + raise TenantError("slug_taken", status_code=409) from exc + self._after_commit(invalidate=[None]) + return tenant + async def set_status(self, tenant_id: str, status: TenantStatus) -> Tenant: """Suspend or reactivate — the call a billing module makes on dunning.""" tenant = await self._require(tenant_id) 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..ab5d52008 --- /dev/null +++ b/modules/tenants/tests-js/tenants_members_pagination.test.tsx @@ -0,0 +1,79 @@ +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(), reload: 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 }, + 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 }, + ); + vi.unstubAllGlobals(); +}); 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..d9b243cb3 --- /dev/null +++ b/modules/tenants/tests/test_tenants_cache_singleflight.py @@ -0,0 +1,65 @@ +"""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 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) 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..33c874ac9 --- /dev/null +++ b/modules/tenants/tests/test_tenants_members_pagination.py @@ -0,0 +1,95 @@ +"""Bounded, tenant-scoped member pages with stable ordering and complete traversal.""" + +from datetime import UTC, datetime + +import pytest +from sqlalchemy import select +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["page"], props["per_page"], props["total"]) == (6, 10, 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): + from sqlalchemy import delete + + 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["page"], props["per_page"], props["total"]) == (1, 1, 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(): + from fastapi import FastAPI + from tenants.endpoints.api import router + + 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..4e78cd35c --- /dev/null +++ b/modules/tenants/tests/test_tenants_negative_host_cache.py @@ -0,0 +1,127 @@ +"""Unknown hosts expire quickly and committed tenant changes invalidate misses.""" + +from contextlib import asynccontextmanager +from types import SimpleNamespace + +from cachetools import TTLCache +from fastapi import FastAPI +from tenants import host_resolver + + +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]), + raising=False, + ) + reads = 0 + + class DB: + async def execute(self, stmt): + nonlocal reads + reads += 1 + return SimpleNamespace(first=lambda: None) + + @asynccontextmanager + async def session(): + yield DB() + + app = FastAPI() + app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + 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_create_invalidates_negative_host_cache_after_commit(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 + assert (await host_resolver.tenant_for_slug(app, "new-org"))[0] == response.json()["id"] + + +async def test_rename_invalidates_old_hit_and_new_miss_only_after_commit(app, tenant_client): + from simple_module_db.transaction import finalize_session + from tenants.contracts.schemas import TenantCreate + from tenants.resolver import make_invalidator + from tenants.service import TenantService + + async with tenant_client("owner") as (_, tenant_id, _): + async with app.state.sm.db.session_factory() as db: + service = TenantService(db, invalidate=make_invalidator(app)) + tenant = await service.get(tenant_id) + old_slug = tenant.slug + assert (await host_resolver.tenant_for_slug(app, old_slug))[0] == tenant_id + assert await host_resolver.tenant_for_slug(app, "renamed") is None + await service.rename_tenant(tenant_id, TenantCreate(name="Renamed", slug="renamed")) + assert await host_resolver.tenant_for_slug(app, "renamed") is None + await finalize_session(db) + assert await host_resolver.tenant_for_slug(app, old_slug) is None + assert (await host_resolver.tenant_for_slug(app, "renamed"))[0] == tenant_id + + +async def test_host_invalidation_during_read_retries_before_caching(): + import asyncio + + 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) + + @asynccontextmanager + async def session(): + yield DB() + + app = FastAPI() + app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + task = asyncio.create_task(host_resolver.tenant_for_slug(app, "racing")) + await started.wait() + host_resolver.forget_hosts() + 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_rename_rollback_and_duplicate_slug_preserve_tenant(app, tenant_client): + import pytest + from tenants.contracts.schemas import TenantCreate + from tenants.errors import TenantError + from tenants.models import Tenant + from tenants.resolver import make_invalidator + from tenants.service import TenantService + + async with ( + tenant_client("owner") as (_, tenant_id, _), + app.state.sm.db.session_factory() as db, + ): + original = await db.get(Tenant, tenant_id) + original_name, original_slug = original.name, original.slug + db.add(Tenant(name="Taken", slug="taken")) + await db.commit() + service = TenantService(db, invalidate=make_invalidator(app)) + renamed = await service.rename_tenant(tenant_id, TenantCreate(name="Name only")) + assert (renamed.name, renamed.slug) == ("Name only", original_slug) + await db.rollback() + with pytest.raises(TenantError) as error: + await service.rename_tenant(tenant_id, TenantCreate(name="Conflict", slug="taken")) + assert error.value.status_code == 409 + await db.rollback() + tenant = await service.get(tenant_id) + assert (tenant.name, tenant.slug) == (original_name, original_slug) From 2e238e0db0124afcfa496b3a68d7a2649db392a8 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 18:07:10 +0100 Subject: [PATCH 2/5] test(tenants): narrow optional resolver results before indexing Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tenants/tests/test_tenants_negative_host_cache.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/modules/tenants/tests/test_tenants_negative_host_cache.py b/modules/tenants/tests/test_tenants_negative_host_cache.py index 4e78cd35c..38084be1a 100644 --- a/modules/tenants/tests/test_tenants_negative_host_cache.py +++ b/modules/tenants/tests/test_tenants_negative_host_cache.py @@ -46,7 +46,8 @@ async def test_create_invalidates_negative_host_cache_after_commit(app, user_cli 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 - assert (await host_resolver.tenant_for_slug(app, "new-org"))[0] == response.json()["id"] + hit = await host_resolver.tenant_for_slug(app, "new-org") + assert hit is not None and hit[0] == response.json()["id"] async def test_rename_invalidates_old_hit_and_new_miss_only_after_commit(app, tenant_client): @@ -60,13 +61,15 @@ async def test_rename_invalidates_old_hit_and_new_miss_only_after_commit(app, te service = TenantService(db, invalidate=make_invalidator(app)) tenant = await service.get(tenant_id) old_slug = tenant.slug - assert (await host_resolver.tenant_for_slug(app, old_slug))[0] == tenant_id + hit = await host_resolver.tenant_for_slug(app, old_slug) + assert hit is not None and hit[0] == tenant_id assert await host_resolver.tenant_for_slug(app, "renamed") is None await service.rename_tenant(tenant_id, TenantCreate(name="Renamed", slug="renamed")) assert await host_resolver.tenant_for_slug(app, "renamed") is None await finalize_session(db) assert await host_resolver.tenant_for_slug(app, old_slug) is None - assert (await host_resolver.tenant_for_slug(app, "renamed"))[0] == tenant_id + hit = await host_resolver.tenant_for_slug(app, "renamed") + assert hit is not None and hit[0] == tenant_id async def test_host_invalidation_during_read_retries_before_caching(): From d3d0d74cbddcd56aba19e314770cac35eb881af8 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 18:14:12 +0100 Subject: [PATCH 3/5] fix(tenants): scope tenant-creation invalidation, bound cache-fill retries; optimize cleanups Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 2 +- docs/framework/invalidation.md | 4 +- docs/modules/tenants.md | 16 +- .../core/simple_module_core/singleflight.py | 4 + modules/permissions/permissions/grants.py | 17 ++- .../test_permissions_grants_singleflight.py | 27 ++++ modules/tenants/tenants/endpoints/api.py | 13 +- modules/tenants/tenants/endpoints/views.py | 4 +- modules/tenants/tenants/host_resolver.py | 54 +++++-- modules/tenants/tenants/pages/Members.tsx | 15 +- modules/tenants/tenants/resolver.py | 38 +++-- modules/tenants/tenants/service.py | 16 +- .../tenants_members_pagination.test.tsx | 7 +- .../tests/test_tenants_cache_singleflight.py | 25 +++ .../tests/test_tenants_members_pagination.py | 13 +- .../tests/test_tenants_negative_host_cache.py | 143 ++++++++++-------- 16 files changed, 245 insertions(+), 153 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7538d6dbc..6b3495d53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,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 invalidate host lookups after committed tenant creation or renaming (2026-11-05 local performance review). +- 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 and the owner's memberships after committed tenant creation; other users' caches are not cleared. Retries after concurrent invalidation are bounded to three reads (2026-11-05 local performance review). - Include module `ui/` sources in wheel-installed Tailwind scanning and the asset manifest; retain legacy `components/` support. ## [0.0.43] — 2026-11-04 diff --git a/docs/framework/invalidation.md b/docs/framework/invalidation.md index 517a50cf0..18f2f7480 100644 --- a/docs/framework/invalidation.md +++ b/docs/framework/invalidation.md @@ -122,7 +122,9 @@ lock, so a waiter can retry without a cached failed task. The helper does not version or invalidate cached data. Authorization caches must also compare their invalidation epoch before storing **or returning** a DB read, -and retry against a fresh session if the epoch changed. Tenant memberships and +and retry against a fresh session if the epoch changed. Bound the retries +(`MAX_FILL_ATTEMPTS`, 3): after that, return the last read uncached rather than +loop while invalidations keep arriving. Tenant memberships and direct permission grants do this; their existing TTLs still bound staleness from missed cross-worker invalidations. The users session-version cache has not yet adopted the helper. diff --git a/docs/modules/tenants.md b/docs/modules/tenants.md index 0460040f0..796107474 100644 --- a/docs/modules/tenants.md +++ b/docs/modules/tenants.md @@ -154,19 +154,21 @@ 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. No stale read is -returned to avoid waiting. The 60-second membership TTL and invalidation bus +when waiting ends, and retry if invalidated during a DB read. After three reads +that each raced an invalidation, the request uses the latest read without caching +it; that read started after every invalidation the request observed. The 60-second membership TTL and invalidation bus remain the fallback for changes in other workers. Permission grants use the same shared single-flight helper with their existing 30-second TTL. Unknown subdomain slugs are negatively cached for five seconds (bounded to 10,000 -entries); known slugs keep their 60-second TTL. Committed tenant creation, -`TenantService.rename_tenant(tenant_id, TenantCreate(...))`, status changes and -deletion invalidate host lookups via the tenant invalidator. Renaming also clears -cached membership names/slugs. Construct services with the normal invalidator +entries); known slugs keep their 60-second TTL. Committed tenant creation +drops only the new slug's host lookups (key `host_invalidation_key(slug)` on the +tenant invalidation channel, so other workers drop it too) and the owner's +memberships; other users' caches stay warm. Status changes and deletion clear all +host lookups and memberships. Construct services with the normal invalidator (`make_invalidator(app)` outside request dependencies); direct SQL writes must publish the tenant invalidation channel with `key=None` after commit or wait for -TTL expiry. No new rename endpoint is exposed. +TTL expiry. ## Models diff --git a/framework/core/simple_module_core/singleflight.py b/framework/core/simple_module_core/singleflight.py index 591e395a4..2ac7a5c6d 100644 --- a/framework/core/simple_module_core/singleflight.py +++ b/framework/core/simple_module_core/singleflight.py @@ -11,6 +11,10 @@ from contextlib import asynccontextmanager from dataclasses import dataclass, field +# Epoch-checked fills retry at most this many reads; continuous invalidation +# must not keep a request looping. +MAX_FILL_ATTEMPTS = 3 + @dataclass class _Flight: diff --git a/modules/permissions/permissions/grants.py b/modules/permissions/permissions/grants.py index 9cf7ffab6..91e041f83 100644 --- a/modules/permissions/permissions/grants.py +++ b/modules/permissions/permissions/grants.py @@ -24,7 +24,7 @@ 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 SingleFlight +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS, SingleFlight from sqlalchemy import select from permissions.models import UserPermission @@ -56,7 +56,8 @@ """ _CACHES: WeakKeyDictionary = WeakKeyDictionary() -_FLIGHTS: WeakKeyDictionary = WeakKeyDictionary() +_FLIGHTS: WeakKeyDictionary[FastAPI, SingleFlight] = WeakKeyDictionary() +# Bumped by every invalidation so a read started earlier never caches stale grants. _epoch = 0 @@ -73,10 +74,7 @@ def _cache(app: FastAPI) -> TTLCache: def clear_grants_cache() -> None: """Empty the cache — for tests that need a cold read.""" - global _epoch - _epoch += 1 - for cache in list(_CACHES.values()): - cache.clear() + apply_invalidation(Invalidation(CHANNEL)) def _user_uuid(raw: Any) -> uuid.UUID | None: @@ -98,12 +96,14 @@ async def direct_grant_source(request: Request, user: Any) -> frozenset[str]: if user_id is None: return frozenset() grants = _cache(request.app) + # One ``get``, not ``in`` then ``[]``: a TTLCache entry can expire between. cached = grants.get(user_id, _MISS) if cached is not _MISS: return cached flight = _FLIGHTS.setdefault(request.app, SingleFlight()) async with flight.hold(user_id): - while True: + keys: frozenset[str] = frozenset() + for _ in range(MAX_FILL_ATTEMPTS): cached = grants.get(user_id, _MISS) if cached is not _MISS: return cached @@ -116,6 +116,9 @@ async def direct_grant_source(request: Request, user: Any) -> frozenset[str]: if started == _epoch: grants[user_id] = keys return keys + # Invalidated during every read: the last read started after the last + # invalidation this request saw, so answer with it but cache nothing. + return keys def apply_invalidation(invalidation: Invalidation, app: FastAPI | None = None) -> None: diff --git a/modules/permissions/tests/test_permissions_grants_singleflight.py b/modules/permissions/tests/test_permissions_grants_singleflight.py index 421f9ad30..63596e3b4 100644 --- a/modules/permissions/tests/test_permissions_grants_singleflight.py +++ b/modules/permissions/tests/test_permissions_grants_singleflight.py @@ -10,6 +10,7 @@ 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 @@ -57,3 +58,29 @@ async def session(): 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/tenants/endpoints/api.py b/modules/tenants/tenants/endpoints/api.py index ce1ddaf1e..275d396a9 100644 --- a/modules/tenants/tenants/endpoints/api.py +++ b/modules/tenants/tenants/endpoints/api.py @@ -44,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() @@ -109,17 +110,17 @@ async def switch_tenant( # ── active tenant: members ───────────────────────────────────── +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=[ - MemberView(user_id=m.user_id, email=m.email, role=m.role, joined_at=m.created_at) - for m in page.items - ], + **page.model_dump(exclude={"items"}), items=[_member_view(m) for m in page.items] ) @@ -128,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 fc3bee188..4c978c64c 100644 --- a/modules/tenants/tenants/endpoints/views.py +++ b/modules/tenants/tenants/endpoints/views.py @@ -95,9 +95,7 @@ async def members( } for m in page.items ], - "page": page.page, - "per_page": page.per_page, - "total": page.total, + "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}, }, diff --git a/modules/tenants/tenants/host_resolver.py b/modules/tenants/tenants/host_resolver.py index 32285da99..106228431 100644 --- a/modules/tenants/tenants/host_resolver.py +++ b/modules/tenants/tenants/host_resolver.py @@ -23,6 +23,7 @@ from cachetools import TTLCache from fastapi import FastAPI from simple_module_core.app_state import get_public_routes +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS from sqlalchemy import select from starlette.requests import Request @@ -31,9 +32,17 @@ HOST_TTL_SECONDS = 60 NEGATIVE_HOST_TTL_SECONDS = 5 +# Membership keys are user ids; this prefix marks a key that names one slug. +HOST_KEY_PREFIX = "host:" _MISSING_SLUGS: TTLCache[str, bool] = TTLCache(maxsize=10_000, ttl=NEGATIVE_HOST_TTL_SECONDS) -_epoch = 0 _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 host_invalidation_key(slug: str) -> str: + """Invalidation key that drops one slug's host lookups (see ``resolver.forget``).""" + return f"{HOST_KEY_PREFIX}{slug}" def forget_hosts() -> None: @@ -43,6 +52,13 @@ def forget_hosts() -> None: _MISSING_SLUGS.clear() +def forget_slug(slug: str) -> None: + global _epoch + _epoch += 1 + _BY_SLUG.pop(slug, None) + _MISSING_SLUGS.pop(slug, None) + + def subdomains_enabled(request: Request) -> bool: return bool(request.app.state.tenants.settings.subdomain_base.strip().strip(".")) @@ -59,25 +75,32 @@ 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.""" - while True: + row = None + for _ in range(MAX_FILL_ATTEMPTS): hit = _BY_SLUG.get(slug) if hit is not None: return hit if _MISSING_SLUGS.get(slug): return None started = _epoch - 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 started != _epoch: - continue - if row is None: - _MISSING_SLUGS[slug] = True - return None - found = (row[0], row[1]) - _BY_SLUG[slug] = found - return found + row = await _read_slug(app, slug) + if started == _epoch: + if row is None: + _MISSING_SLUGS[slug] = True + else: + _BY_SLUG[slug] = row + return row + # Invalidated during every read: the last read started after the last + # invalidation this request saw, so answer with it but cache nothing. + return row + + +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() + return None if row is None else (row[0], row[1]) def is_public_route(request: Request) -> bool: @@ -96,7 +119,10 @@ async def resolve_from_host(request: Request, slug: str) -> tuple[str | None, bo __all__ = [ + "HOST_KEY_PREFIX", "forget_hosts", + "forget_slug", + "host_invalidation_key", "is_public_route", "resolve_from_host", "subdomain_slug", diff --git a/modules/tenants/tenants/pages/Members.tsx b/modules/tenants/tenants/pages/Members.tsx index 5c0c07975..b9891d554 100644 --- a/modules/tenants/tenants/pages/Members.tsx +++ b/modules/tenants/tenants/pages/Members.tsx @@ -19,6 +19,12 @@ interface Seats { limit: number | null; } +interface Pagination { + page: number; + per_page: number; + total: number; +} + interface Props { tenant: Tenant; my_role: MembershipRole; @@ -27,9 +33,7 @@ interface Props { members: Member[]; invitations: Invitation[]; seats: Seats; - page: number; - per_page: number; - total: number; + pagination: Pagination; } function Members({ @@ -40,11 +44,10 @@ function Members({ members, invitations, seats, - page, - per_page, - total, + pagination, }: Props) { const { t } = useT(); + const { page, per_page, total } = pagination; function reload() { visitPage(page); diff --git a/modules/tenants/tenants/resolver.py b/modules/tenants/tenants/resolver.py index 47042d2ee..8a4563332 100644 --- a/modules/tenants/tenants/resolver.py +++ b/modules/tenants/tenants/resolver.py @@ -24,7 +24,7 @@ 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 SingleFlight +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS, SingleFlight 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 @@ -38,7 +38,9 @@ ) from tenants.contracts.schemas import MyTenantView from tenants.host_resolver import ( + HOST_KEY_PREFIX, forget_hosts, + forget_slug, is_public_route, resolve_from_host, subdomain_slug, @@ -68,6 +70,9 @@ def forget(key: str | None) -> None: global _epoch + if key is not None and key.startswith(HOST_KEY_PREFIX): + forget_slug(key.removeprefix(HOST_KEY_PREFIX)) # a new slug: memberships unaffected + return _epoch += 1 if key is None: _MEMBERSHIPS.clear() @@ -104,27 +109,30 @@ async def memberships_for(app: FastAPI, user_id: str) -> list[MyTenantView]: return cached flight = _FLIGHTS.setdefault(app, SingleFlight()) async with flight.hold(user_id): - while True: + views: list[MyTenantView] = [] + for _ in range(MAX_FILL_ATTEMPTS): cached = _MEMBERSHIPS.get(user_id) if cached is not None: return cached started = _epoch - async with app.state.sm.db.session_factory() as db: - rows = await TenantService(db).list_for_user(user_id) - views = [ - 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 - ] + views = await _read_memberships(app, user_id) if _epoch == started: _MEMBERSHIPS[user_id] = views return views + # Invalidated during every read: the last read started after the last + # invalidation this request saw, so answer with it but cache nothing. + return views + + +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) + 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 + ] 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 71cd0605b..aeb0380bb 100644 --- a/modules/tenants/tenants/service.py +++ b/modules/tenants/tenants/service.py @@ -41,6 +41,7 @@ ) from tenants.contracts.schemas import TenantCreate from tenants.errors import TenantError +from tenants.host_resolver import host_invalidation_key from tenants.models import Membership, Tenant logger = logging.getLogger(__name__) @@ -136,23 +137,10 @@ async def create_tenant( self._after_commit( TenantCreated(tenant.id, tenant.slug, tenant.name, owner_user_id), MembershipAdded(tenant.id, owner_user_id, MembershipRole.OWNER), - invalidate=[None], + invalidate=[owner_user_id, host_invalidation_key(tenant.slug)], ) return tenant - async def rename_tenant(self, tenant_id: str, data: TenantCreate) -> Tenant: - """Rename an organisation; slug changes drop host and membership caches on commit.""" - tenant = await self.lock(tenant_id) - tenant.name = data.name - if data.slug is not None: - tenant.slug = data.slug - try: - await self.db.flush() - except IntegrityError as exc: - raise TenantError("slug_taken", status_code=409) from exc - self._after_commit(invalidate=[None]) - return tenant - async def set_status(self, tenant_id: str, status: TenantStatus) -> Tenant: """Suspend or reactivate — the call a billing module makes on dunning.""" tenant = await self._require(tenant_id) diff --git a/modules/tenants/tests-js/tenants_members_pagination.test.tsx b/modules/tenants/tests-js/tenants_members_pagination.test.tsx index ab5d52008..e547573b7 100644 --- a/modules/tenants/tests-js/tenants_members_pagination.test.tsx +++ b/modules/tenants/tests-js/tenants_members_pagination.test.tsx @@ -9,7 +9,7 @@ import Members from '../tenants/pages/Members'; vi.mock('@inertiajs/react', () => ({ Head: () => null, usePage: () => ({ url: '/tenants/members' }), - router: { get: vi.fn(), reload: vi.fn() }, + router: { get: vi.fn() }, })); configureI18n({ locale: 'en', @@ -36,9 +36,7 @@ const props = { members: [{ user_id: 'member', email: 'member@x.io', role: 'member' as const, joined_at: null }], invitations: [], seats: { used: 60, limit: null }, - page: 2, - per_page: 25, - total: 60, + pagination: { page: 2, per_page: 25, total: 60 }, }; test('page navigation keeps server page size and uses server clamped page', async () => { @@ -75,5 +73,4 @@ test('role mutation reloads the current page and page size', async () => { { page: 2, per_page: 25 }, { preserveState: true, preserveScroll: true }, ); - vi.unstubAllGlobals(); }); diff --git a/modules/tenants/tests/test_tenants_cache_singleflight.py b/modules/tenants/tests/test_tenants_cache_singleflight.py index d9b243cb3..853134fcc 100644 --- a/modules/tenants/tests/test_tenants_cache_singleflight.py +++ b/modules/tenants/tests/test_tenants_cache_singleflight.py @@ -7,6 +7,7 @@ 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 @@ -63,3 +64,27 @@ async def read(self, user_id): ) * 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_members_pagination.py b/modules/tenants/tests/test_tenants_members_pagination.py index 33c874ac9..3da58e6a6 100644 --- a/modules/tenants/tests/test_tenants_members_pagination.py +++ b/modules/tenants/tests/test_tenants_members_pagination.py @@ -3,7 +3,9 @@ from datetime import UTC, datetime import pytest -from sqlalchemy import select +from fastapi import FastAPI +from sqlalchemy import delete, select +from tenants.endpoints.api import router from tenants.models import Membership, Tenant @@ -43,7 +45,7 @@ async def test_members_api_and_page_paginate_and_clamp(app, tenant_client): props = ( await client.get("/tenants/members?page=99&per_page=10", headers={"X-Inertia": "true"}) ).json()["props"] - assert (props["page"], props["per_page"], props["total"]) == (6, 10, 60) + 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()[ @@ -63,15 +65,13 @@ async def test_members_api_rejects_invalid_pagination(tenant_client, query): async def test_empty_roster_and_lenient_page_parameters(app, tenant_client): - from sqlalchemy import delete - 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["page"], props["per_page"], props["total"]) == (1, 1, 1) + 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: @@ -82,9 +82,6 @@ async def test_empty_roster_and_lenient_page_parameters(app, tenant_client): def test_members_openapi_preserves_typed_items(): - from fastapi import FastAPI - from tenants.endpoints.api import router - application = FastAPI() application.include_router(router) schema = application.openapi() diff --git a/modules/tenants/tests/test_tenants_negative_host_cache.py b/modules/tenants/tests/test_tenants_negative_host_cache.py index 38084be1a..e8c711664 100644 --- a/modules/tenants/tests/test_tenants_negative_host_cache.py +++ b/modules/tenants/tests/test_tenants_negative_host_cache.py @@ -1,20 +1,34 @@ -"""Unknown hosts expire quickly and committed tenant changes invalidate misses.""" +"""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 tenants import host_resolver +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 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]), - raising=False, + host_resolver, "_MISSING_SLUGS", TTLCache(maxsize=10_000, ttl=5, timer=lambda: now[0]) ) reads = 0 @@ -24,12 +38,7 @@ async def execute(self, stmt): reads += 1 return SimpleNamespace(first=lambda: None) - @asynccontextmanager - async def session(): - yield DB() - - app = FastAPI() - app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + app = _fake_app(DB()) for _ in range(50): assert await host_resolver.tenant_for_slug(app, "missing") is None assert reads == 1 @@ -41,7 +50,7 @@ async def session(): assert reads == 3 -async def test_create_invalidates_negative_host_cache_after_commit(app, user_client): +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"}) @@ -50,31 +59,45 @@ async def test_create_invalidates_negative_host_cache_after_commit(app, user_cli assert hit is not None and hit[0] == response.json()["id"] -async def test_rename_invalidates_old_hit_and_new_miss_only_after_commit(app, tenant_client): - from simple_module_db.transaction import finalize_session - from tenants.contracts.schemas import TenantCreate - from tenants.resolver import make_invalidator - from tenants.service import TenantService - - async with tenant_client("owner") as (_, tenant_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=make_invalidator(app)) - tenant = await service.get(tenant_id) - old_slug = tenant.slug - hit = await host_resolver.tenant_for_slug(app, old_slug) - assert hit is not None and hit[0] == tenant_id - assert await host_resolver.tenant_for_slug(app, "renamed") is None - await service.rename_tenant(tenant_id, TenantCreate(name="Renamed", slug="renamed")) - assert await host_resolver.tenant_for_slug(app, "renamed") is None + service = TenantService(db, invalidate=resolver.make_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) - assert await host_resolver.tenant_for_slug(app, old_slug) is None - hit = await host_resolver.tenant_for_slug(app, "renamed") - assert hit is not None and hit[0] == tenant_id + 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 + + +def test_host_key_from_the_bus_drops_one_slug_and_no_memberships(): + resolver._MEMBERSHIPS["someone"] = [] + host_resolver._BY_SLUG["kept"] = ("kept-id", "active") + host_resolver._MISSING_SLUGS["created"] = True + key = host_resolver.host_invalidation_key("created") + resolver._apply_invalidation(Invalidation(INVALIDATION_CHANNEL, key=key)) + assert "created" not in host_resolver._MISSING_SLUGS + assert host_resolver._BY_SLUG.get("kept") == ("kept-id", "active") + assert resolver.cached_memberships("someone") == [] async def test_host_invalidation_during_read_retries_before_caching(): - import asyncio - started, release = asyncio.Event(), asyncio.Event() reads = 0 @@ -87,44 +110,32 @@ async def execute(self, stmt): await release.wait() return SimpleNamespace(first=lambda: row) - @asynccontextmanager - async def session(): - yield DB() - - app = FastAPI() - app.state.sm = SimpleNamespace(db=SimpleNamespace(session_factory=session)) + app = _fake_app(DB()) task = asyncio.create_task(host_resolver.tenant_for_slug(app, "racing")) await started.wait() - host_resolver.forget_hosts() + 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_rename_rollback_and_duplicate_slug_preserve_tenant(app, tenant_client): - import pytest - from tenants.contracts.schemas import TenantCreate - from tenants.errors import TenantError - from tenants.models import Tenant - from tenants.resolver import make_invalidator - from tenants.service import TenantService - - async with ( - tenant_client("owner") as (_, tenant_id, _), - app.state.sm.db.session_factory() as db, - ): - original = await db.get(Tenant, tenant_id) - original_name, original_slug = original.name, original.slug - db.add(Tenant(name="Taken", slug="taken")) - await db.commit() - service = TenantService(db, invalidate=make_invalidator(app)) - renamed = await service.rename_tenant(tenant_id, TenantCreate(name="Name only")) - assert (renamed.name, renamed.slug) == ("Name only", original_slug) - await db.rollback() - with pytest.raises(TenantError) as error: - await service.rename_tenant(tenant_id, TenantCreate(name="Conflict", slug="taken")) - assert error.value.status_code == 409 - await db.rollback() - tenant = await service.get(tenant_id) - assert (tenant.name, tenant.slug) == (original_name, original_slug) +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 From bee37dc1a2e2c7741039b0c77246e16a6b9c330e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 18:17:35 +0100 Subject: [PATCH 4/5] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/simple_module_core/singleflight.py | 4 +++- modules/api_tokens/tests/test_api_tokens_hardening.py | 2 +- modules/permissions/permissions/grants.py | 3 ++- modules/tenants/tenants/resolver.py | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/framework/core/simple_module_core/singleflight.py b/framework/core/simple_module_core/singleflight.py index 2ac7a5c6d..f7d5099c6 100644 --- a/framework/core/simple_module_core/singleflight.py +++ b/framework/core/simple_module_core/singleflight.py @@ -28,7 +28,9 @@ def __init__(self) -> None: @asynccontextmanager async def hold(self, key: Hashable) -> AsyncGenerator[None]: - flight = self._locks.setdefault(key, _Flight()) + flight = self._locks.get(key) + if flight is None: + flight = self._locks[key] = _Flight() flight.users += 1 try: async with flight.lock: diff --git a/modules/api_tokens/tests/test_api_tokens_hardening.py b/modules/api_tokens/tests/test_api_tokens_hardening.py index a4095e018..c1106ec79 100644 --- a/modules/api_tokens/tests/test_api_tokens_hardening.py +++ b/modules/api_tokens/tests/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 91e041f83..5902fe326 100644 --- a/modules/permissions/permissions/grants.py +++ b/modules/permissions/permissions/grants.py @@ -100,7 +100,8 @@ async def direct_grant_source(request: Request, user: Any) -> frozenset[str]: cached = grants.get(user_id, _MISS) if cached is not _MISS: return cached - flight = _FLIGHTS.setdefault(request.app, SingleFlight()) + if (flight := _FLIGHTS.get(request.app)) is None: + flight = _FLIGHTS[request.app] = SingleFlight() async with flight.hold(user_id): keys: frozenset[str] = frozenset() for _ in range(MAX_FILL_ATTEMPTS): diff --git a/modules/tenants/tenants/resolver.py b/modules/tenants/tenants/resolver.py index 8a4563332..ade5d7d66 100644 --- a/modules/tenants/tenants/resolver.py +++ b/modules/tenants/tenants/resolver.py @@ -107,7 +107,8 @@ async def memberships_for(app: FastAPI, user_id: str) -> list[MyTenantView]: cached = _MEMBERSHIPS.get(user_id) if cached is not None: return cached - flight = _FLIGHTS.setdefault(app, SingleFlight()) + if (flight := _FLIGHTS.get(app)) is None: + flight = _FLIGHTS[app] = SingleFlight() async with flight.hold(user_id): views: list[MyTenantView] = [] for _ in range(MAX_FILL_ATTEMPTS): From 9a85bfb98e41b83db9b85386b3b4d822d8ff4ac9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 18:21:59 +0100 Subject: [PATCH 5/5] =?UTF-8?q?fix:=20address=20code=20review=20findings?= =?UTF-8?q?=20(round=201,=20pass=201b)=20=E2=80=94=20hosts=20channel,=20sh?= =?UTF-8?q?ared=20fill=20helper,=20single=20member=20count?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 4 +- docs/framework/invalidation.md | 36 +++++++-- docs/modules/tenants.md | 39 ++++++--- .../core/simple_module_core/singleflight.py | 63 ++++++++++++++- .../core/tests/test_core_singleflight.py | 47 ++++++++++- modules/permissions/permissions/grants.py | 32 +++----- modules/tenants/tenants/_membership_ops.py | 20 +++-- modules/tenants/tenants/constants.py | 4 + modules/tenants/tenants/deps.py | 2 + modules/tenants/tenants/endpoints/views.py | 4 +- modules/tenants/tenants/host_resolver.py | 80 ++++++++++++------- modules/tenants/tenants/module.py | 2 + modules/tenants/tenants/resolver.py | 35 ++++---- modules/tenants/tenants/service.py | 11 ++- .../tests/test_tenants_negative_host_cache.py | 26 ++++-- 15 files changed, 296 insertions(+), 109 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b3495d53..ba02cf906 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,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. +- **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. - Scaffold new modules with `core/` and `web/` areas and mirrored nested Python tests; existing modules migrate individually. See `docs/module-layout.md`. - `mail` follows the module layout: `mail.core.{constants,settings,state,data_compatibility}` and `mail.delivery.{sender,runtime}`. `mail.settings` and `mail.constants` still import, with a `DeprecationWarning`, until 0.0.46. - `site_lock` follows the module layout: `site_lock.core.{constants,settings,state}` and `site_lock.web.{middleware,page,rate_limit}` (the unlock template moves with the page). `site_lock.settings` still imports, with a `DeprecationWarning`, until 0.0.46. @@ -25,7 +25,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 and the owner's memberships after committed tenant creation; other users' caches are not cleared. Retries after concurrent invalidation are bounded to three reads (2026-11-05 local performance review). +- 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). - Include module `ui/` sources in wheel-installed Tailwind scanning and the asset manifest; retain legacy `components/` support. ## [0.0.43] — 2026-11-04 diff --git a/docs/framework/invalidation.md b/docs/framework/invalidation.md index 18f2f7480..7c9a2aa0d 100644 --- a/docs/framework/invalidation.md +++ b/docs/framework/invalidation.md @@ -120,11 +120,31 @@ 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 helper does not version or invalidate cached data. Authorization caches must -also compare their invalidation epoch before storing **or returning** a DB read, -and retry against a fresh session if the epoch changed. Bound the retries -(`MAX_FILL_ATTEMPTS`, 3): after that, return the last read uncached rather than -loop while invalidations keep arriving. Tenant memberships and -direct permission grants do this; their existing TTLs still bound staleness from -missed cross-worker invalidations. The users session-version cache has not yet -adopted the helper. +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 796107474..87d81e723 100644 --- a/docs/modules/tenants.md +++ b/docs/modules/tenants.md @@ -155,20 +155,35 @@ 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 uses the latest read without caching -it; that read started after every invalidation the request observed. The 60-second membership TTL and invalidation bus -remain the fallback for changes in other workers. Permission grants use the same -shared single-flight helper with their existing 30-second TTL. +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. Committed tenant creation -drops only the new slug's host lookups (key `host_invalidation_key(slug)` on the -tenant invalidation channel, so other workers drop it too) and the owner's -memberships; other users' caches stay warm. Status changes and deletion clear all -host lookups and memberships. Construct services with the normal invalidator -(`make_invalidator(app)` outside request dependencies); direct SQL writes must -publish the tenant invalidation channel with `key=None` after commit or wait for -TTL expiry. +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 diff --git a/framework/core/simple_module_core/singleflight.py b/framework/core/simple_module_core/singleflight.py index f7d5099c6..b9172043c 100644 --- a/framework/core/simple_module_core/singleflight.py +++ b/framework/core/simple_module_core/singleflight.py @@ -1,21 +1,76 @@ -"""Serialize cache fills per key; callers recheck their cache inside the lock. +"""Serialize cache fills per key, and fill caches without storing stale reads. -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. +``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, Hashable +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) diff --git a/framework/core/tests/test_core_singleflight.py b/framework/core/tests/test_core_singleflight.py index 05e47990a..a6566e519 100644 --- a/framework/core/tests/test_core_singleflight.py +++ b/framework/core/tests/test_core_singleflight.py @@ -3,7 +3,7 @@ import asyncio import pytest -from simple_module_core.singleflight import SingleFlight +from simple_module_core.singleflight import MAX_FILL_ATTEMPTS, MISS, SingleFlight, fill @pytest.mark.parametrize("cancel_leader", [False, True]) @@ -53,3 +53,48 @@ async def waiter(): 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/permissions/permissions/grants.py b/modules/permissions/permissions/grants.py index 5902fe326..9e56261f4 100644 --- a/modules/permissions/permissions/grants.py +++ b/modules/permissions/permissions/grants.py @@ -24,7 +24,7 @@ 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 MAX_FILL_ATTEMPTS, SingleFlight +from simple_module_core.singleflight import MISS, SingleFlight, fill from sqlalchemy import select from permissions.models import UserPermission @@ -69,9 +69,6 @@ def _cache(app: FastAPI) -> TTLCache: return cache -_MISS = object() - - def clear_grants_cache() -> None: """Empty the cache — for tests that need a cold read.""" apply_invalidation(Invalidation(CHANNEL)) @@ -97,29 +94,26 @@ async def direct_grant_source(request: Request, user: Any) -> frozenset[str]: return frozenset() grants = _cache(request.app) # One ``get``, not ``in`` then ``[]``: a TTLCache entry can expire between. - cached = grants.get(user_id, _MISS) - if cached is not _MISS: + cached = grants.get(user_id, MISS) + if cached is not MISS: return cached if (flight := _FLIGHTS.get(request.app)) is None: flight = _FLIGHTS[request.app] = SingleFlight() async with flight.hold(user_id): - keys: frozenset[str] = frozenset() - for _ in range(MAX_FILL_ATTEMPTS): - cached = grants.get(user_id, _MISS) - if cached is not _MISS: - return cached - started = _epoch + + 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) ) - keys = frozenset(result.scalars().all()) - if started == _epoch: - grants[user_id] = keys - return keys - # Invalidated during every read: the last read started after the last - # invalidation this request saw, so answer with it but cache nothing. - return keys + 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) + ) def apply_invalidation(invalidation: Invalidation, app: FastAPI | None = None) -> None: diff --git a/modules/tenants/tenants/_membership_ops.py b/modules/tenants/tenants/_membership_ops.py index 3c1b61023..e3c338f5d 100644 --- a/modules/tenants/tenants/_membership_ops.py +++ b/modules/tenants/tenants/_membership_ops.py @@ -33,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: ... @@ -68,15 +69,20 @@ async def list_members(self, tenant_id: str, params: PageParams) -> Page[Members ) 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 e8d6c8517..f5940fe09 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/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/views.py b/modules/tenants/tenants/endpoints/views.py index 4c978c64c..f85188acc 100644 --- a/modules/tenants/tenants/endpoints/views.py +++ b/modules/tenants/tenants/endpoints/views.py @@ -78,6 +78,8 @@ async def members( ) 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, @@ -97,7 +99,7 @@ async def members( ], "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 106228431..a54404114 100644 --- a/modules/tenants/tenants/host_resolver.py +++ b/modules/tenants/tenants/host_resolver.py @@ -20,31 +20,27 @@ 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.singleflight import MAX_FILL_ATTEMPTS +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 -# Membership keys are user ids; this prefix marks a key that names one slug. -HOST_KEY_PREFIX = "host:" _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 host_invalidation_key(slug: str) -> str: - """Invalidation key that drops one slug's host lookups (see ``resolver.forget``).""" - return f"{HOST_KEY_PREFIX}{slug}" - - def forget_hosts() -> None: global _epoch _epoch += 1 @@ -59,6 +55,35 @@ def forget_slug(slug: str) -> 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: return bool(request.app.state.tenants.settings.subdomain_base.strip().strip(".")) @@ -74,25 +99,23 @@ 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.""" - row = None - for _ in range(MAX_FILL_ATTEMPTS): + """``(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 - if _MISSING_SLUGS.get(slug): - return None - started = _epoch - row = await _read_slug(app, slug) - if started == _epoch: - if row is None: - _MISSING_SLUGS[slug] = True - else: - _BY_SLUG[slug] = row - return row - # Invalidated during every read: the last read started after the last - # invalidation this request saw, so answer with it but cache nothing. - return row + 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: @@ -119,12 +142,13 @@ async def resolve_from_host(request: Request, slug: str) -> tuple[str | None, bo __all__ = [ - "HOST_KEY_PREFIX", + "forget_host", "forget_hosts", "forget_slug", - "host_invalidation_key", "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 422576cc7..22c3c3611 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/resolver.py b/modules/tenants/tenants/resolver.py index ade5d7d66..06f9083b5 100644 --- a/modules/tenants/tenants/resolver.py +++ b/modules/tenants/tenants/resolver.py @@ -24,7 +24,7 @@ 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 MAX_FILL_ATTEMPTS, SingleFlight +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 @@ -38,9 +38,7 @@ ) from tenants.contracts.schemas import MyTenantView from tenants.host_resolver import ( - HOST_KEY_PREFIX, forget_hosts, - forget_slug, is_public_route, resolve_from_host, subdomain_slug, @@ -69,10 +67,12 @@ 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 - if key is not None and key.startswith(HOST_KEY_PREFIX): - forget_slug(key.removeprefix(HOST_KEY_PREFIX)) # a new slug: memberships unaffected - return _epoch += 1 if key is None: _MEMBERSHIPS.clear() @@ -110,19 +110,16 @@ async def memberships_for(app: FastAPI, user_id: str) -> list[MyTenantView]: if (flight := _FLIGHTS.get(app)) is None: flight = _FLIGHTS[app] = SingleFlight() async with flight.hold(user_id): - views: list[MyTenantView] = [] - for _ in range(MAX_FILL_ATTEMPTS): - cached = _MEMBERSHIPS.get(user_id) - if cached is not None: - return cached - started = _epoch - views = await _read_memberships(app, user_id) - if _epoch == started: - _MEMBERSHIPS[user_id] = views - return views - # Invalidated during every read: the last read started after the last - # invalidation this request saw, so answer with it but cache nothing. - return views + + 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]: diff --git a/modules/tenants/tenants/service.py b/modules/tenants/tenants/service.py index aeb0380bb..211703882 100644 --- a/modules/tenants/tenants/service.py +++ b/modules/tenants/tenants/service.py @@ -41,7 +41,6 @@ ) from tenants.contracts.schemas import TenantCreate from tenants.errors import TenantError -from tenants.host_resolver import host_invalidation_key from tenants.models import Membership, Tenant logger = logging.getLogger(__name__) @@ -65,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]]] = [] @@ -79,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,7 +141,8 @@ async def create_tenant( self._after_commit( TenantCreated(tenant.id, tenant.slug, tenant.name, owner_user_id), MembershipAdded(tenant.id, owner_user_id, MembershipRole.OWNER), - invalidate=[owner_user_id, host_invalidation_key(tenant.slug)], + invalidate=[owner_user_id], + invalidate_hosts=[tenant.slug], ) return tenant diff --git a/modules/tenants/tests/test_tenants_negative_host_cache.py b/modules/tenants/tests/test_tenants_negative_host_cache.py index e8c711664..e33820c3b 100644 --- a/modules/tenants/tests/test_tenants_negative_host_cache.py +++ b/modules/tenants/tests/test_tenants_negative_host_cache.py @@ -10,7 +10,7 @@ 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 INVALIDATION_CHANNEL +from tenants.constants import HOSTS_INVALIDATION_CHANNEL, INVALIDATION_CHANNEL from tenants.contracts.schemas import TenantCreate from tenants.service import TenantService @@ -62,7 +62,11 @@ async def test_previously_missing_slug_resolves_right_after_create(app, user_cli 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)) + 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 @@ -86,15 +90,27 @@ async def test_create_keeps_other_users_memberships_and_slugs(app, tenant_client assert host_resolver._BY_SLUG.get(existing) == cached_host -def test_host_key_from_the_bus_drops_one_slug_and_no_memberships(): +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 - key = host_resolver.host_invalidation_key("created") - resolver._apply_invalidation(Invalidation(INVALIDATION_CHANNEL, key=key)) + 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():