Tech-debt batch 28: lower-priority checklist — deprecations, menu permissions, flag cleanup, invite errors, audit filters, lease clock (#698)

CHANGELOG.md

@@ -25,6 +25,18 @@ All notable changes to this project are documented in this file. The format is b
   distinguishing an unconfigured worker from single-tenant operation (#685).
 
 ### Fixed
+- Deprecate `auth.deps.require_permission` for removal in 0.1.0; recommend
+  `RequiresPermission` and enforce the removal release in the shim ledger (#698).
+- **Behaviour change:** gate Files and Branding navigation by their page
+  permissions instead of hardcoded roles (#698).
+- Clear deleted tenants' feature-flag registry overrides locally and on remote
+  tenant-deletion invalidations, preserving other scopes (#639, #698).
+- Keep unexpected bulk-invite exception details in logs and return a generic
+  failure message instead of exposing SQL and parameters (#698).
+- **Behaviour change:** align audit-log JSON list actor search and inclusive
+  end-of-day filtering with the browse screen and CSV export (#698).
+- Start database lease TTLs after locking each slot on Postgres, and recheck expiry after
+  lock waits when refreshing or releasing (#698).
 - File storage: cancellation during upload-session lookup prevents transport from
   starting; clearing completed upload rows releases their retained files (#695, #588).
 - **Behaviour change:** password resets revoke all browser sessions, access tokens

docs/framework/leases.md

@@ -50,7 +50,7 @@ The backend is process-global because a Celery worker has no `app.state`. `creat
 | Configured | Backend | Notes |
 |---|---|---|
 | `SM_REDIS_URL` | `RedisLeaseBackend` (`simple_module_hosting`) | `SET NX PX` plus Lua compare-and-set for refresh and release; a semaphore is a sorted set scored by expiry against Redis `TIME`; `INCR <key>:fence` for the fence. |
-| a database, no Redis | `DbLeaseBackend` (`simple_module_db`) | Rows in `sm_lease (name, slot, holder, fence, expires_at)`, primary key `(name, slot)`. Every comparison uses the database clock, so clock skew between hosts does not matter. |
+| a database, no Redis | `DbLeaseBackend` (`simple_module_db`) | Rows in `sm_lease (name, slot, holder, fence, expires_at)`, primary key `(name, slot)`. Every comparison uses the database clock, so clock skew between hosts does not matter; on Postgres the clock is read after the slot row is locked, so a lock wait never shortens a TTL. |
 | neither | `InProcessLeaseBackend` | One process only. Also what tests and eager Celery get when nothing else is configured. |
 
 Backend errors propagate. A lease that the backend could not confirm is never reported as held. This is unlike the rate limiter, which falls back to per-process counters.

docs/modules/audit_log.md

@@ -111,6 +111,13 @@ GET /api/audit_log/
 
 Requires `audit_log.view`. Returns `AuditEntryList`. Query filters (all optional): `entity_type`, `entity_id`, `action`, `user_id`, `from_date`, `to_date`, `page` (≥1), `page_size` (1–200, default 50). Results are ordered newest-first (`created_at` desc).
 
+The JSON list, browse screen and CSV export share actor and date-range semantics
+(#698): `user_id` accepts an exact UUID or a name/email substring (an exact
+stored actor id when the directory cannot search). `to_date` includes the whole
+day it names, even if supplied with a time. `from_date` remains the supplied
+lower bound. Service callers needing an exact upper timestamp can use
+`AuditLogService.list_entries` or construct `EntryFilters` directly.
+
 ### View
 
 | Method + path | Inertia component | Permission |

docs/modules/branding.md

@@ -306,3 +306,7 @@ edits show at once, in every worker once a transport is installed.
 
 - Branding has no table: the system identity is SYSTEM-scope settings, a tenant's overrides are TENANT-scope settings (see [Per-tenant branding](#per-tenant-branding)).
 - The primary colour overrides the `--primary` / `--sidebar-primary` CSS variables from a single hex; the full OKLCH colour scale is not regenerated.
+
+The Branding navigation entry requires `branding.view`, matching its page guard,
+rather than the `admin` role (#698). Custom roles with that permission see the
+entry; changing branding still requires `branding.manage`.

docs/modules/feature_flags.md

@@ -132,3 +132,10 @@ Top-level keys in `feature_flags/locales/en.json`:
 - The `FeatureFlagService` keeps the in-memory registry in sync after every DB mutation, so `is_enabled()` is always cheap.
 - Tenant-scope endpoints use a non-empty `scope_id`; the system scope uses `""`. Don't pass `None` — the unique index treats `NULL` as distinct on Postgres and you'll end up with duplicate rows.
 - The `Browse` page lists tenants that have any override attached, so you can jump from system view → a tenant view that actually has overrides without typing IDs.
+
+Tenant deletion durably removes persisted tenant overrides and clears the local
+registry through a `TenantDeleted` subscriber (#639, #698). Workers also forget
+the tenant on the existing `tenants.deleted` invalidation broadcast; system and
+other tenants' overrides are preserved, including overrides for unknown flags.
+The periodic override reload remains the fallback for missed broadcasts after
+durable cleanup completes.

docs/modules/file_storage.md

@@ -449,3 +449,7 @@ and worker processes, and verify authenticated/public downloads, ranged reads,
 thumbnails and deletion before resuming writes. Keep the source and backups
 until verification is complete, then remove source objects explicitly. There
 is no automatic migration command or historical-backend registry.
+
+The Files navigation entry requires `file_storage.download`, matching the browse
+route (#698). It is available to permitted ordinary users on single-tenant
+installs as well as tenant members; role names do not independently gate it.

docs/modules/users.md

@@ -420,6 +420,10 @@ Trade-offs to know about:
 
 `mailer/` ships one implementation of the `Mailer` protocol, `TemplatedMailer`. It renders the bundled verify / reset / invite templates and sends each as a `MailMessage` through the framework mail sender (`simple_module_core.mail.current_mail_sender()`), looked up at send time. The transport (SMTP or console), its credentials and the `mail.mailer` health check belong to the [`mail` module](./mail.md); the former `mailer` and `smtp_*` users settings no longer exist.
 
+Bulk invite failures return a known, safe reason for an existing account and a
+generic retry message for unexpected errors (#698). SQL statements, parameters
+and backend exception messages stay in the application log.
+
 `delivers_email` reflects the installed sender, so screens that need to hand the one-time link back (reset-sent card, add people, resend and bulk invite) do so whenever the sender only logs. `build_mailer(settings)` returns the `TemplatedMailer`; a third-party `Mailer` can still be assigned to `app.state.users.mailer`.
 
 ## OAuth providers

framework/core/simple_module_core/feature_flags.py

@@ -71,6 +71,12 @@ def clear_override(self, name: str, tenant_id: str | None = None) -> None:
         else:
             self._tenant_overrides.pop((name, tenant_id), None)
 
+    def clear_tenant_overrides(self, tenant_id: str) -> None:
+        """Forget every override for a deleted tenant, including unknown flags."""
+        self._tenant_overrides = {
+            key: value for key, value in self._tenant_overrides.items() if key[1] != tenant_id
+        }
+
     def replace_overrides(
         self, system: dict[str, bool], tenants: dict[tuple[str, str], bool]
     ) -> None:

framework/db/simple_module_db/leases.py

@@ -106,19 +106,40 @@ async def _tx(self) -> AsyncGenerator[tuple[AsyncConnection, datetime]]:
                 await conn.execute(_SQLITE_WRITE_LOCK)
             yield conn, await _db_now(conn)
 
+    async def _locked_now(self, conn: AsyncConnection, *where: Any) -> datetime:
+        """Lock the matching rows on Postgres, then read the clock (#698).
+
+        Postgres computes an UPDATE's new values before it waits on a row lock,
+        so a time read first could grant (or "refresh") a lease that expired
+        during the wait. SQLite holds the database write lock already.
+        """
+        await conn.execute(select(_T.c.slot).where(*where).with_for_update())
+        return await _db_now(conn)
+
     async def _take_slot(
         self,
         conn: AsyncConnection,
         name: str,
         slot: int,
         token: str,
-        expires: datetime,
+        ttl: float,
         now: datetime,
     ) -> int | None:
         """Claim *slot* if free (missing or expired) or already ours; return its fence."""
-        values = {"holder": token, "expires_at": expires}
-        ins = _insert(conn.dialect.name).values(name=name, slot=slot, fence=1, **values)
-        taken = (await conn.execute(ins.on_conflict_do_nothing())).rowcount == 1
+        if self._sqlite:
+            values = {"holder": token, "expires_at": now + timedelta(seconds=ttl)}
+            ins = _insert(conn.dialect.name).values(name=name, slot=slot, fence=1, **values)
+            taken = (await conn.execute(ins.on_conflict_do_nothing())).rowcount == 1
+        else:
+            # An expired placeholder gives a missing slot a row to lock; the
+            # UPDATE below then claims it exactly like an expired lease (fence 1).
+            placeholder = _insert(conn.dialect.name).values(
+                name=name, slot=slot, holder=_FREE, fence=0, expires_at=_EPOCH
+            )
+            await conn.execute(placeholder.on_conflict_do_nothing())
+            now = await self._locked_now(conn, _row(name, slot))
+            values = {"holder": token, "expires_at": now + timedelta(seconds=ttl)}
+            taken = False
         if not taken:
             stmt = (
                 update(_T)
@@ -144,15 +165,14 @@ async def _next_counter(self, conn: AsyncConnection, name: str) -> int:
 
     async def acquire(self, name: str, token: str, ttl: float) -> int | None:
         async with self._tx() as (conn, now):
-            return await self._take_slot(conn, name, 0, token, now + timedelta(seconds=ttl), now)
+            return await self._take_slot(conn, name, 0, token, ttl, now)
 
     async def semaphore_acquire(self, name: str, token: str, slots: int, ttl: float) -> int | None:
         async with self._tx() as (conn, now):
-            expires = now + timedelta(seconds=ttl)
             mine = await self._slot_of(conn, name, token, now)
             order = [mine] if mine is not None else range(slots)
             for slot in order:
-                if await self._take_slot(conn, name, slot, token, expires, now) is not None:
+                if await self._take_slot(conn, name, slot, token, ttl, now) is not None:
                     return await self._next_counter(conn, name)
             return None
 
@@ -169,8 +189,11 @@ async def _held_update(
     ) -> bool:
         """Extend (``ttl``) or end (``None``) *token*'s unexpired hold."""
         async with self._tx() as (conn, now):
-            where = [_T.c.name == name, _T.c.holder == token, _T.c.expires_at > now]
+            where = [_T.c.name == name, _T.c.holder == token]
             where.append(_T.c.slot >= 0 if any_slot else _T.c.slot == 0)
+            if not self._sqlite:
+                now = await self._locked_now(conn, *where)
+            where.append(_T.c.expires_at > now)
             expires = now if ttl is None else now + timedelta(seconds=ttl)
             result = await conn.execute(update(_T).where(*where).values(expires_at=expires))
             return result.rowcount > 0

framework/db/tests/test_db_lease_lock_wait.py

@@ -0,0 +1,39 @@
+"""A lease taken after waiting on a row lock is still valid when granted (#698)."""
+
+import asyncio
+import uuid
+
+import pytest
+from simple_module_db.leases import DbLeaseBackend, LeaseRow
+from sqlalchemy import func, select
+
+_T = LeaseRow.__table__
+
+
+@pytest.mark.postgres
+@pytest.mark.parametrize("operation", ["acquire", "refresh"])
+async def test_postgres_lock_wait_does_not_consume_ttl(engine, operation):
+    async with engine.begin() as conn:
+        await conn.run_sync(LeaseRow.metadata.create_all)
+    backend = DbLeaseBackend(engine)
+    name = f"lockwait-{uuid.uuid4().hex}"
+    holder = "old" if operation == "acquire" else "me"
+    ttl = 0.01 if operation == "acquire" else 60
+    assert await backend.acquire(name, holder, ttl) is not None
+    await asyncio.sleep(0.05)  # intentional-sleep: let the 10 ms lease expire
+
+    async with engine.connect() as blocker:
+        await blocker.begin()
+        row = (_T.c.name == name) & (_T.c.slot == 0)
+        await blocker.execute(select(_T.c.fence).where(row).with_for_update())
+        if operation == "acquire":
+            pending = asyncio.create_task(backend.acquire(name, "new", 1.0))
+        else:
+            pending = asyncio.create_task(backend.refresh(name, "me", 1.0))
+        await asyncio.sleep(1.5)  # intentional-sleep: hold the lock past the 1 s TTL
+        await blocker.commit()
+    assert await pending
+
+    async with engine.connect() as conn:
+        live = await conn.execute(select(_T.c.expires_at > func.clock_timestamp()).where(row))
+        assert live.scalar_one() is True

host/tests/test_deprecation_expiry.py

@@ -21,6 +21,7 @@
 
 # Modules whose import registers a deprecation (shim modules and call-site shims).
 _SHIM_MODULES = (
+    "auth.deps",
     "host.routes_legacy",
     "permissions.deps",
     "settings.registration",
@@ -31,6 +32,7 @@
     "simple_module_core.versioning",
 )
 _EXPECTED = {
+    "auth.deps.require_permission",
     "host.routes_legacy (pre-/admin view URL redirects)",
     "permissions.deps.invalidate_grants_on_commit",
     "settings.registration",

modules/audit_log/audit_log/endpoints/api.py

@@ -26,6 +26,7 @@
 
 @router.get("/", response_model=AuditEntryList, dependencies=_VIEW)
 async def list_audit_entries(
+    request: Request,
     service: AuditLogServiceDep,
     entity_type: str | None = Query(default=None),
     entity_id: str | None = Query(default=None),
@@ -38,18 +39,17 @@ async def list_audit_entries(
     page: int = Query(default=1, ge=1, le=MAX_PAGE),
     page_size: int = Query(default=DEFAULT_PAGE_SIZE, ge=1, le=MAX_PER_PAGE),
 ) -> AuditEntryList:
-    return await service.list_entries(
+    filters = EntryFilters.for_date_only_range(
         entity_type=entity_type,
         entity_id=entity_id,
         action=action,
-        user_id=user_id,
+        actor_match=actor_filter(get_user_directory(request.app), user_id or ""),
         correlation_id=correlation_id,
         tenant_id=tenant_id,
         from_date=from_date,
         to_date=to_date,
-        page=page,
-        page_size=page_size,
     )
+    return await service.list_filtered(filters, page=page, page_size=page_size)
 
 
 @router.get("/export.csv", response_model=None, dependencies=_VIEW)

modules/audit_log/audit_log/filters.py

@@ -34,10 +34,10 @@ def end_of_day(moment: datetime | None) -> datetime | None:
 class EntryFilters:
     """A narrowing of the audit log, independent of paging.
 
-    ``user_id`` is an exact match on the stored id — what the JSON list
-    endpoint takes. ``actor_match`` is the predicate an Actor *search* compiles
-    to (see ``resolve.actor_filter``), which may match by name or email and is
-    therefore a subquery rather than a value.
+    ``user_id`` is an exact match on the stored id for service callers.
+    ``actor_match`` is the predicate an Actor search compiles to (see
+    ``resolve.actor_filter``), which may match by name or email and is therefore
+    a subquery rather than a value.
     """
 
     entity_type: str | None = None
@@ -68,12 +68,11 @@ def for_date_only_range(
 
         The upper bound is stretched over the whole day it names — see
         :func:`end_of_day`. A named constructor rather than a step a caller has
-        to remember: the browse view and the CSV export both build filters from
+        to remember: the browse view, JSON list and CSV export build filters from
         the same query string, and one of them forgetting would mean the file
         and the page disagreed about which day the range ended on.
 
-        Plain construction keeps exact ``datetime`` bounds, which is what the
-        JSON list endpoint's documented ``from_date``/``to_date`` mean.
+        Plain construction keeps exact ``datetime`` bounds for service callers.
         """
         return cls(
             entity_type=entity_type,

modules/audit_log/tests/test_audit_log_api_filter_parity.py

@@ -0,0 +1,48 @@
+"""JSON list and CSV export interpret the same query identically (#698)."""
+
+import csv
+import io
+import uuid
+from datetime import UTC, datetime
+
+import pytest
+from audit_log.constants import ACTION_UPDATED
+from audit_log.models import AuditEntry
+from users.models import User
+
+
+@pytest.mark.parametrize("actor", ["", "Filter Person", "filter-person@example.com", "id"])
+async def test_audit_log_list_matches_export_actor_and_day(app, authenticated_client, actor):
+    user_id = uuid.uuid4()
+    async with app.state.sm.db.session_factory() as db:
+        db.add(
+            User(
+                id=user_id,
+                email="filter-person@example.com",
+                full_name="Filter Person",
+                hashed_password="unused",
+            )
+        )
+        await db.flush()
+        for entity_id, day in [("included", 19), ("excluded", 20)]:
+            db.add(
+                AuditEntry(
+                    entity_type="FilterParity",
+                    entity_id=entity_id,
+                    action=ACTION_UPDATED,
+                    changes=[],
+                    user_id=str(user_id),
+                    created_at=datetime(2026, 8, day, 14, tzinfo=UTC),
+                )
+            )
+        await db.commit()
+    params = {
+        "entity_type": "FilterParity",
+        "to_date": "2026-08-19",
+        "user_id": str(user_id) if actor == "id" else actor,
+    }
+    listing = await authenticated_client.get("/api/audit_log/", params=params)
+    export = await authenticated_client.get("/api/audit_log/export.csv", params=params)
+    assert listing.status_code == export.status_code == 200
+    assert [row["entity_id"] for row in csv.DictReader(io.StringIO(export.text))] == ["included"]
+    assert [row["entity_id"] for row in listing.json()["items"]] == ["included"]

modules/audit_log/tests/test_date_range_filter.py

@@ -44,8 +44,7 @@ def test_the_screen_constructor_stretches_the_upper_bound(self) -> None:
         assert filters.to_date == datetime(2026, 8, 19, 23, 59, 59, 999999, tzinfo=UTC)
 
     def test_plain_construction_keeps_the_exact_bound(self) -> None:
-        """The JSON list endpoint documents ``to_date`` as a timestamp; only
-        the screen's date-only control needs stretching."""
+        """Service callers can request an exact bound through plain construction."""
         exact = datetime(2026, 8, 19, 10, 30, tzinfo=UTC)
 
         assert EntryFilters(to_date=exact).to_date == exact