fix: tenancy-adoption review follow-ups (permissions editor, branding reaper)

framework/core/simple_module_core/permissions.py

@@ -64,6 +64,9 @@ class PermissionRegistry:
     def __init__(self) -> None:
         self._groups: dict[str, PermissionGroup] = {}
         self._role_map: dict[str, set[str]] = {}
+        # Admin-editor grants (DB-backed). Kept apart from ``_role_map`` (code
+        # mappings) so replacing a role's DB keys never drops a code grant.
+        self._role_overlay: dict[str, set[str]] = {}
         self._all_permissions_cache: list[str] | None = None
         self._role_map_cache: dict[str, list[str]] | None = None
 
@@ -116,18 +119,26 @@ def map_role(self, role: str, permissions: list[str]) -> None:
         self._role_map[role].update(permissions)
         self._invalidate()
 
+    def set_role_overlay(self, role: str, permissions: Collection[str]) -> None:
+        """Replace *role*'s persisted (admin-editor) grants.
+
+        Unlike :meth:`map_role` this is a replace, so removals take effect; the
+        code-registered mappings from :meth:`map_role` are left untouched and
+        stay merged into :attr:`role_map`.
+        """
+        self._role_overlay[role] = set(permissions)
+        self._invalidate()
+
     @property
     def role_map(self) -> dict[str, list[str]]:
         """Merged role→permission mapping (``DEFAULT_ROLE_PERMISSIONS`` + module maps)."""
         if self._role_map_cache is None:
             merged: dict[str, list[str]] = {
                 role: list(perms) for role, perms in DEFAULT_ROLE_PERMISSIONS.items()
             }
-            for role, perms in self._role_map.items():
-                if role in merged:
-                    merged[role] = list(set(merged[role]) | perms)
-                else:
-                    merged[role] = list(perms)
+            for source in (self._role_map, self._role_overlay):
+                for role, perms in source.items():
+                    merged[role] = list(set(merged.get(role, ())) | perms)
             self._role_map_cache = merged
         return self._role_map_cache
 

modules/branding/branding/reaper.py

@@ -72,8 +72,12 @@ async def reap(app: FastAPI, file_id: str, *, tenant_id: str | None) -> None:
                     return
                 services = app.state.file_storage
                 storage = FileStorageService(db, services.backend, services.settings)
-                await storage.delete(parsed, platform=tenant_id is None)
+                # Soft-delete and commit first, drop the bytes after: a failed
+                # commit must never leave a live row with no object. A failure
+                # after the commit only orphans an object, which is acceptable.
+                row = await storage.delete(parsed, platform=tenant_id is None, drop_object=False)
                 await db.commit()
+                await storage.drop_object(row)
     except Exception:
         # Deliberately broad: a cleanup failure is never a reason to fail (or,
         # running after commit, to misreport) a rebrand that succeeded.

modules/branding/branding/tenant_branding.py

@@ -188,7 +188,11 @@ def _shared_read(
     entry = (cache.epoch, read)
     cache.inflight[tenant_id] = entry
 
-    def _done(_: asyncio.Future[dict[str, str]]) -> None:
+    def _done(fut: asyncio.Future[dict[str, str]]) -> None:
+        # Retrieve the outcome so a failed read whose every waiter was
+        # cancelled never logs "exception was never retrieved".
+        if not fut.cancelled():
+            fut.exception()
         if cache.inflight.get(tenant_id) is entry:
             del cache.inflight[tenant_id]
 

modules/branding/tests/test_asset_lifecycle.py

@@ -47,7 +47,11 @@ async def fake_upload(
             return row
 
         async def fake_delete(
-            self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False
+            self: FileStorageService,
+            file_id: uuid.UUID,
+            *,
+            platform: bool = False,
+            drop_object: bool = True,
         ) -> StoredFile:
             assert platform, "branding must act on platform-owned files"
             if file_id in store.deleted:
@@ -65,6 +69,7 @@ async def fake_delete(
 
         monkeypatch.setattr(FileStorageService, "upload", fake_upload, raising=True)
         monkeypatch.setattr(FileStorageService, "delete", fake_delete, raising=True)
+        monkeypatch.setattr(FileStorageService, "drop_object", _noop_drop, raising=True)
 
 
 @pytest.fixture
@@ -143,7 +148,11 @@ async def test_a_failed_cleanup_does_not_fail_the_rebrand(
     await _upload(authenticated_client, "logo")
 
     async def boom(
-        self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False
+        self: FileStorageService,
+        file_id: uuid.UUID,
+        *,
+        platform: bool = False,
+        drop_object: bool = True,
     ) -> StoredFile:
         raise RuntimeError("backend unavailable")
 
@@ -153,3 +162,7 @@ async def boom(
 
     assert resp.status_code == 200, resp.text
     assert resp.json()["logo_url"] is None
+
+
+async def _noop_drop(self: FileStorageService, row: StoredFile) -> None:
+    """The fake rows have no backend object to drop."""

modules/branding/tests/test_branding_reap_ordering.py

@@ -0,0 +1,110 @@
+"""The reaper commits the soft-delete before it drops the backend object (#396).
+
+Also: the tenant-branding in-flight read map is per app and its failures are
+always retrieved.
+"""
+
+from __future__ import annotations
+
+import asyncio
+import gc
+
+from branding import reaper, tenant_branding
+from branding.tenant_branding import TenantCache
+from file_storage.models import StoredFile
+from settings.contracts.schemas import SettingScope
+from settings.service import SettingService
+from simple_module_db import all_tenants
+from sqlalchemy import select
+from sqlalchemy.ext.asyncio import AsyncSession
+
+_PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 32
+
+
+async def _upload_then_unset(app, a) -> str:
+    resp = await a.client.post(
+        "/api/branding/tenant/logo", files={"file": ("l.png", _PNG, "image/png")}
+    )
+    assert resp.status_code == 200, resp.text
+    async with app.state.sm.db.session_factory() as db:
+        service = SettingService(db)
+        row = await service.get_scoped(SettingScope.TENANT, a.tenant_id, "branding.logo_file_id")
+        file_id = row.value
+        await service.delete_scoped(SettingScope.TENANT, a.tenant_id, "branding.logo_file_id")
+        await db.commit()
+    return file_id
+
+
+async def _row(app, file_id: str) -> StoredFile:
+    with all_tenants():
+        async with app.state.sm.db.session_factory() as db:
+            stmt = select(StoredFile).execution_options(include_deleted=True)
+            return next(r for r in (await db.execute(stmt)).scalars() if str(r.id) == file_id)
+
+
+async def test_failed_commit_keeps_the_bytes(app, tenant_client, monkeypatch):
+    async with tenant_client() as a:
+        file_id = await _upload_then_unset(app, a)
+        key = (await _row(app, file_id)).key
+        backend = app.state.file_storage.backend
+
+        async def boom(self):
+            raise RuntimeError("commit failed")
+
+        monkeypatch.setattr(AsyncSession, "commit", boom)
+        await reaper.reap(app, file_id, tenant_id=a.tenant_id)  # logs, never raises
+        monkeypatch.undo()
+
+        assert (await _row(app, file_id)).is_deleted is False
+        assert await backend.exists(key)
+
+
+async def test_bytes_are_dropped_only_after_the_commit(app, tenant_client, monkeypatch):
+    events: list[str] = []
+    orig_commit = AsyncSession.commit
+
+    async def commit(self):
+        events.append("commit")
+        await orig_commit(self)
+
+    async with tenant_client() as a:
+        file_id = await _upload_then_unset(app, a)
+        backend = app.state.file_storage.backend
+        orig_delete = backend.delete
+
+        async def delete(key):
+            events.append("delete")
+            await orig_delete(key)
+
+        monkeypatch.setattr(AsyncSession, "commit", commit)
+        monkeypatch.setattr(backend, "delete", delete)
+        await reaper.reap(app, file_id, tenant_id=a.tenant_id)
+
+    assert events == ["commit", "delete"]
+    assert (await _row(app, file_id)).is_deleted is True
+
+
+async def test_failed_unawaited_read_is_still_retrieved(app, monkeypatch):
+    cache = TenantCache()
+    started = asyncio.Event()
+
+    async def boom(_app, _tid):
+        started.set()
+        raise RuntimeError("read failed")
+
+    monkeypatch.setattr(tenant_branding, "read_overrides", boom)
+    entry = tenant_branding._shared_read(app, cache, "t1")
+    await started.wait()
+    await asyncio.sleep(0)
+    assert entry[1].done()
+    assert cache.inflight == {}
+
+    seen: list[dict] = []
+    loop = asyncio.get_running_loop()
+    loop.set_exception_handler(lambda _l, ctx: seen.append(ctx))
+    try:
+        del entry
+        gc.collect()
+        assert not seen
+    finally:
+        loop.set_exception_handler(None)

modules/branding/tests/test_logo_dark.py

@@ -58,14 +58,19 @@ async def body() -> AsyncIterator[bytes]:
         return StreamDownload(file=rows[file_id], body=body())
 
     async def fake_delete(
-        self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False
+        self: FileStorageService,
+        file_id: uuid.UUID,
+        *,
+        platform: bool = False,
+        drop_object: bool = True,
     ) -> StoredFile:
         assert platform, "branding must act on platform-owned files"
         return rows.pop(file_id)
 
     monkeypatch.setattr(FileStorageService, "upload", fake_upload, raising=True)
     monkeypatch.setattr(FileStorageService, "download", fake_download, raising=True)
     monkeypatch.setattr(FileStorageService, "delete", fake_delete, raising=True)
+    monkeypatch.setattr(FileStorageService, "drop_object", _noop_drop, raising=True)
     return rows
 
 
@@ -177,3 +182,7 @@ async def test_an_unset_dark_logo_is_a_404(client: httpx.AsyncClient) -> None:
     # The frontend never requests it in this state — it falls back to logoUrl —
     # but a hand-typed URL must not 500.
     assert (await client.get("/api/branding/logo-dark")).status_code == 404
+
+
+async def _noop_drop(self: FileStorageService, row: StoredFile) -> None:
+    """The fake rows have no backend object to drop."""

modules/file_storage/file_storage/service.py

@@ -243,7 +243,15 @@ async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]:
                 )
         return rows
 
-    async def delete(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile:
+    async def delete(
+        self, file_id: uuid.UUID, *, platform: bool = False, drop_object: bool = True
+    ) -> StoredFile:
+        """Soft-delete the row and (by default) drop the backend object.
+
+        ``drop_object=False`` only flushes the soft-delete: a caller that must
+        commit first (so a failed commit never leaves a live row with no bytes)
+        then calls :meth:`drop_object` with the returned row.
+        """
         # One scope around read + write: its opening flush runs before the
         # platform row is touched, so the guard never sees that change.
         async with platform_scope(self.db, platform):
@@ -253,10 +261,15 @@ async def delete(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredF
             row.is_deleted = True
             row.deleted_at = datetime.now(UTC)
             await self.db.flush()
-        # Object is acceptably absent — eg. a previous delete partially succeeded.
+        if drop_object:
+            await self.drop_object(row)
+        return row
+
+    async def drop_object(self, row: StoredFile) -> None:
+        """Delete ``row``'s backend object; an already-absent object is fine."""
+        # Acceptably absent — eg. a previous delete partially succeeded.
         with contextlib.suppress(StorageNotFoundError):
             await self.backend.delete(row.key)
-        return row
 
 
 def _generate_key(tenant_id: str, filename: str) -> str:

modules/permissions/permissions/service.py

@@ -112,10 +112,9 @@ async def set_role_permissions(
         )
         await self.db.flush()
 
-        # `map_role` is additive — reset the role entry so removals take effect
-        # without a restart. No public replace API on PermissionRegistry yet.
-        self.registry._role_map.pop(role.name, None)
-        self.registry.map_role(role.name, sorted(wanted))
+        # Replace only the DB overlay: code-registered mappings stay in effect
+        # and removed DB keys drop immediately, without a restart.
+        self.registry.set_role_overlay(role.name, wanted)
 
         return RolePermissionsOut(role=role, permissions=sorted(wanted))
 
@@ -273,7 +272,7 @@ async def load_all_into_registry(self) -> None:
         for name, key in result.all():
             by_role.setdefault(name, []).append(key)
         for name, keys in by_role.items():
-            self.registry.map_role(name, keys)
+            self.registry.set_role_overlay(name, keys)
 
     async def sync_admin_all_permissions(self, assigned_by: str | None = None) -> None:
         """Ensure the admin role holds every registered permission key.
@@ -295,4 +294,4 @@ async def sync_admin_all_permissions(self, assigned_by: str | None = None) -> No
             for key in missing
         )
         await self.db.flush()
-        self.registry.map_role(ADMIN_ROLE_NAME, sorted(registered))
+        self.registry.set_role_overlay(ADMIN_ROLE_NAME, registered)

modules/permissions/tests/test_permissions_role_overlay.py

@@ -0,0 +1,43 @@
+"""Saving a role in the editor keeps code-registered grants effective (#396)."""
+
+from __future__ import annotations
+
+import httpx
+from fastapi import FastAPI
+from permissions.constants import PERM_VIEW
+from simple_module_core.permissions import PermissionRegistry
+
+
+def test_overlay_replace_keeps_code_mapping_and_drops_removed_db_keys():
+    reg = PermissionRegistry()
+    reg.map_role("user", ["a.code"])
+    reg.set_role_overlay("user", ["a.db", "b.db"])
+    assert set(reg.role_map["user"]) == {"a.code", "a.db", "b.db"}
+    reg.set_role_overlay("user", ["b.db"])
+    assert set(reg.role_map["user"]) == {"a.code", "b.db"}
+    reg.set_role_overlay("user", [])
+    assert set(reg.role_map["user"]) == {"a.code"}
+
+
+async def test_saving_a_role_keeps_its_code_mapped_grants(
+    authenticated_client: httpx.AsyncClient, app: FastAPI
+):
+    from users.constants import USER_ROLE_ID, USER_ROLE_NAME
+    from users.models import Role
+
+    registry = app.state.sm.permissions
+    registry.add("zz_demo.code_granted")
+    registry.map_role(USER_ROLE_NAME, ["zz_demo.code_granted"])
+    async with app.state.sm.db.session_factory() as db:
+        if await db.get(Role, USER_ROLE_ID) is None:
+            db.add(Role(id=USER_ROLE_ID, name=USER_ROLE_NAME, description="Standard user"))
+            await db.commit()
+
+    for keys in ([PERM_VIEW], []):
+        resp = await authenticated_client.put(
+            f"/api/permissions/roles/{USER_ROLE_ID}", json={"permissions": keys}
+        )
+        assert resp.status_code == 200
+        granted = set(registry.role_map[USER_ROLE_NAME])
+        assert "zz_demo.code_granted" in granted
+        assert (PERM_VIEW in granted) == bool(keys)