Tech-debt batch 24: module settings via their editor; storage settings need restart; Revert after save (#690 #691 #696)

CHANGELOG.md

@@ -37,6 +37,12 @@ All notable changes to this project are documented in this file. The format is b
   validate explicit tenant literals and retain strict isolation and source scoping (#693).
 - Inertia: honor custom once-prop keys in client exclusions, skipping loaders
   and values on full and partial requests (#694).
+- **Behaviour change:** generic settings API and Browse mutations reject registered
+  module keys and point to their validated editor; stop suggesting them (#690).
+- Mark storage backend construction fields as requiring coordinated web/worker
+  restart; document multipart draining and connection-check behaviour (#691).
+- Show Revert immediately after saving a defaulted module field while preserving
+  drafts and the saved confirmation (#696).
 
 ## [0.0.41] — 2026-10-25
 

docs/modules/file_storage.md

@@ -10,6 +10,16 @@ The extras combine as `simple_module_file_storage[s3,tasks]`. Without
 `background_tasks`, uploads still work; completed multipart uploads keep
 `checksum_sha256` null. Streamed uploads compute their checksum inline.
 
+Backend selection, filesystem root, and S3 bucket, region, credentials, endpoints
+and signature version require a coordinated restart of web and worker processes
+(#691). Saving these fields persists configuration; it does not move files or
+rebuild the web backend. Drain or abort multipart uploads and pause storage traffic
+and jobs before changing them, then restart every process before resuming. Workers
+read stored settings per task and may otherwise use the new store before the web
+process restarts. Existing files must be migrated separately when changing roots
+or buckets. The connection check probes the currently active web backend, so it
+continues to test the old configuration until restart.
+
 ## ModuleMeta
 
 | Field | Value |

docs/modules/settings.md

@@ -7,6 +7,11 @@ Two distinct surfaces:
 1. **Generic key/value settings** — anything addressable by a string `key`. Useful for arbitrary config you don't want to wedge into a pydantic class. Edited at `/admin/settings/store`.
 2. **Per-module pydantic settings** — each module registers a `BaseSettings` subclass via `register_module_settings`. Hydrated from the DB at boot, edited at `/admin/settings/`, hot-swapped on save with a `SettingsReloaded` event so dependents (SMTP clients, Celery configs, …) can rebuild.
 
+Generic store mutations reject keys prefixed by a registered module package with
+HTTP 422 and a link to `/admin/settings/` (or its dedicated management
+page). Use the module editor/API for validation, environment pinning and reload;
+Browse autocomplete offers only declared generic keys (#690).
+
 ## ModuleMeta
 
 | Field | Value |
@@ -127,7 +132,7 @@ per-tenant value subscribes and forgets.
 
 ### Generic K/V API (`/api/settings/...`)
 
-All write endpoints require `settings.edit` / `settings.create` / `settings.delete`; reads need `settings.view`. These are platform-operator permissions: no tenant role holds them, and the holder may write any key at system scope or at any tenant's scope. A TENANT `scope_id` must name a real tenant when a tenant-owning module is installed (`simple_module_core.tenancy.tenant_exists`): 404 on the `/tenant/{scope_id}/…` GET/PUT, 422 for a `POST /` body. `DELETE` stays unvalidated so a deleted tenant's leftovers can be cleared. A key declaring `SettingDefinition.clear_via` (a file set through an upload route, which reaps the stored file) is protected inside `SettingService.delete` / `delete_scoped`, so it holds for every scope (SYSTEM, TENANT, USER) and every caller: a generic delete raises `ManagedKeyError`, which the API maps to 422 naming that route (`clear_via` may be a `{SettingScope: route}` mapping when system and tenant rows clear differently; the 422 names the route for the row's scope, via `clear_route`) and the Inertia store screen shows as a toast. Two deliberate ways through: the owner of the key passes `as_owner=True` after taking responsibility for the file (branding's tenant clear route reaps it), and a TENANT row whose tenant no longer exists may be cleared by a platform operator. The guard needs the registry, which `get_setting_service` supplies; a bare `SettingService(db)` guards nothing.
+All write endpoints require `settings.edit` / `settings.create` / `settings.delete`; reads need `settings.view`. These are platform-operator permissions: no tenant role holds them, and the holder may write generic keys at system scope or at any tenant's scope; registered module namespaces must use the module editor/API. A TENANT `scope_id` must name a real tenant when a tenant-owning module is installed (`simple_module_core.tenancy.tenant_exists`): 404 on the `/tenant/{scope_id}/…` GET/PUT, 422 for a `POST /` body. `DELETE` stays unvalidated so a deleted tenant's leftovers can be cleared. A key declaring `SettingDefinition.clear_via` (a file set through an upload route, which reaps the stored file) is protected inside `SettingService.delete` / `delete_scoped`, so it holds for every scope (SYSTEM, TENANT, USER) and every caller: a generic delete raises `ManagedKeyError`, which the API maps to 422 naming that route (`clear_via` may be a `{SettingScope: route}` mapping when system and tenant rows clear differently; the 422 names the route for the row's scope, via `clear_route`) and the Inertia store screen shows as a toast. Two deliberate ways through: the owner of the key passes `as_owner=True` after taking responsibility for the file (branding's tenant clear route reaps it), and a TENANT row whose tenant no longer exists may be cleared by a platform operator. The guard needs the registry, which `get_setting_service` supplies; a bare `SettingService(db)` guards nothing.
 
 | Method + path | Purpose |
 |---|---|

modules/file_storage/file_storage/health.py

@@ -21,7 +21,10 @@
 
 
 def build_backend_check(app: FastAPI):
-    """Return an async check closing over *app* so it re-reads the live backend."""
+    """Return an async check closing over *app* that probes its startup backend.
+
+    Storage settings are restart-required (#691), so that is the backend in use.
+    """
 
     async def check() -> HealthCheckResult:
         services = getattr(app.state, "file_storage", None)

modules/file_storage/file_storage/settings.py

@@ -39,27 +39,33 @@ class FileStorageSettings(DbBackedSettings):
     # reading each field from its bare name — GH #283.
     model_config = SettingsConfigDict(extra="ignore")
 
-    backend: str = constants.DEFAULT_BACKEND
+    backend: str = Field(
+        default=constants.DEFAULT_BACKEND, json_schema_extra={"requires_restart": True}
+    )
 
     # Filesystem backend
     fs_root_path: str = Field(
         default=constants.DEFAULT_FS_ROOT,
-        json_schema_extra={"group": "Filesystem"},
+        json_schema_extra={"group": "Filesystem", "requires_restart": True},
     )
 
     # S3-compatible backend (works with AWS S3, MinIO, R2, etc.)
-    s3_bucket: str = Field(default="", json_schema_extra={"group": "S3"})
-    s3_region: str = Field(default="", json_schema_extra={"group": "S3"})
-    s3_access_key_id: str = Field(default="", json_schema_extra={"group": "S3"})
-    s3_secret_access_key: str = Field(default="", json_schema_extra={"group": "S3"})
+    s3_bucket: str = Field(default="", json_schema_extra={"group": "S3", "requires_restart": True})
+    s3_region: str = Field(default="", json_schema_extra={"group": "S3", "requires_restart": True})
+    s3_access_key_id: str = Field(
+        default="", json_schema_extra={"group": "S3", "requires_restart": True}
+    )
+    s3_secret_access_key: str = Field(
+        default="", json_schema_extra={"group": "S3", "requires_restart": True}
+    )
     s3_endpoint_url: str = Field(
         default="",
-        json_schema_extra={"group": "S3"},
+        json_schema_extra={"group": "S3", "requires_restart": True},
         description="Custom endpoint for MinIO / R2. Blank uses AWS default.",
     )
     s3_public_endpoint_url: str = Field(
         default="",
-        json_schema_extra={"group": "S3"},
+        json_schema_extra={"group": "S3", "requires_restart": True},
         description=(
             "Browser-facing S3 endpoint used to presign direct part uploads, when it "
             "differs from s3_endpoint_url (e.g. an internal MinIO hostname). Blank "
@@ -68,7 +74,7 @@ class FileStorageSettings(DbBackedSettings):
     )
     s3_signature_version: str = Field(
         default="s3v4",
-        json_schema_extra={"group": "S3"},
+        json_schema_extra={"group": "S3", "requires_restart": True},
         description="Presign signature version. Keep s3v4; SigV2 is rejected by most providers.",
     )
     s3_presign_ttl_seconds: int = Field(

modules/file_storage/tests/test_file_storage_restart_settings.py

@@ -0,0 +1,73 @@
+"""Storage location changes require coordinated restarts (#691)."""
+
+import pytest
+
+
+@pytest.mark.parametrize(
+    "name",
+    [
+        "backend",
+        "fs_root_path",
+        "s3_bucket",
+        "s3_region",
+        "s3_access_key_id",
+        "s3_secret_access_key",
+        "s3_endpoint_url",
+        "s3_public_endpoint_url",
+        "s3_signature_version",
+    ],
+)
+def test_backend_construction_fields_advertise_restart(name):
+    from file_storage.settings import FileStorageSettings
+    from settings._module_settings import _field_view
+
+    field = _field_view(name, FileStorageSettings(), "SM_FILE_STORAGE_")
+    assert field.requires_restart is True
+
+
+async def test_health_check_reads_current_backend(tmp_path):
+    from types import SimpleNamespace
+
+    from fastapi import FastAPI
+    from file_storage.backends.filesystem import FilesystemBackend
+    from file_storage.health import build_backend_check
+    from simple_module_core.health import HealthStatus
+
+    app = FastAPI()
+    app.state.file_storage = SimpleNamespace(backend=None)
+    check = build_backend_check(app)
+    assert (await check()).status == HealthStatus.UNHEALTHY
+    app.state.file_storage.backend = FilesystemBackend(root=tmp_path)
+    assert (await check()).status == HealthStatus.HEALTHY
+
+
+async def test_root_change_applies_after_backend_restart(
+    authenticated_client, storage, tmp_path, checksum_calls
+):
+    from file_storage.backends import build_backend
+    from file_storage.backends.filesystem import FilesystemBackend
+
+    old_root = tmp_path / "old"
+    new_root = tmp_path / "new"
+    storage.settings.fs_root_path = str(old_root)
+    storage.backend = FilesystemBackend(root=old_root)
+    original_backend = storage.backend
+    response = await authenticated_client.put(
+        "/api/settings/modules/file_storage", json={"fs_root_path": str(new_root)}
+    )
+    assert response.status_code == 200, response.text
+    assert storage.backend is original_backend
+    response = await authenticated_client.post(
+        "/api/file-storage/upload", files={"file": ("before.txt", b"before restart", "text/plain")}
+    )
+    assert response.status_code == 201, response.text
+    assert [p.read_bytes() for p in old_root.rglob("*.txt")] == [b"before restart"]
+    assert not new_root.exists()
+
+    # Simulate startup constructing the backend from the newly hydrated settings.
+    storage.backend = build_backend(storage.settings)
+    response = await authenticated_client.post(
+        "/api/file-storage/upload", files={"file": ("after.txt", b"after restart", "text/plain")}
+    )
+    assert response.status_code == 201, response.text
+    assert [p.read_bytes() for p in new_root.rglob("*.txt")] == [b"after restart"]

modules/settings/settings/_managed_keys.py

@@ -22,7 +22,7 @@
 
 from collections.abc import Awaitable, Callable
 
-from settings.constants import ERR_MANAGED_KEY_DELETE
+from settings.constants import ERR_MANAGED_KEY_DELETE, ERR_MODULE_KEY_EDIT
 from settings.contracts.registry import SettingsRegistry, clear_route
 from settings.contracts.schemas import SettingScope
 from settings.exceptions import SettingsError
@@ -58,3 +58,11 @@ async def ensure_deletable(
     if orphaned:
         return
     raise ManagedKeyError(key, clear_route(definition, scope))
+
+
+class ModuleKeyError(SettingsError):
+    """A generic mutation would bypass the module editor's validation."""
+
+    def __init__(self, package: str, edit_via: str) -> None:
+        self.edit_via = edit_via
+        super().__init__(ERR_MODULE_KEY_EDIT.format(package=package, edit_via=edit_via))

modules/settings/settings/constants.py

@@ -145,6 +145,7 @@
 ERR_UNKNOWN_TENANT: Final = "Unknown tenant"
 ERR_NO_ACTIVE_TENANT: Final = "No active organisation for this request"
 ERR_MANAGED_KEY_DELETE: Final = "This setting is managed elsewhere; clear it at {clear_via}"
+ERR_MODULE_KEY_EDIT: Final = "Module settings must be edited at {edit_via} (package: {package})"
 ERR_NOT_TENANT_OVERRIDABLE: Final = "This setting cannot be changed per organisation"
 
 # ── HTTP ─────────────────────────────────────────────────────────────

modules/settings/settings/deps.py

@@ -12,6 +12,7 @@
 
 from fastapi import Depends, Request
 from simple_module_core.app_state import get_services
+from simple_module_core.module_settings import get_module_settings_registry
 from simple_module_db.deps import get_db
 from sqlalchemy.ext.asyncio import AsyncSession
 
@@ -36,6 +37,15 @@ async def tenant_is_live(tenant_id: str) -> bool:
     )
 
 
+async def get_generic_setting_service(
+    request: Request,
+    service: SettingService = Depends(get_setting_service),
+) -> SettingService:
+    """Raw store routes must send module configuration through its own editor."""
+    service.module_registry = get_module_settings_registry(request.app)
+    return service
+
+
 def get_settings_registry(request: Request) -> SettingsRegistry:
     """Return the app-wide settings registry populated during boot."""
     services = getattr(request.app.state, MODULE_PACKAGE)

modules/settings/settings/endpoints/_create_form.py

@@ -7,6 +7,7 @@
 from simple_module_hosting.inertia_utils import redirect_back_with_errors, validation_errors_to_dict
 from starlette.responses import RedirectResponse
 
+from settings._managed_keys import ModuleKeyError
 from settings._unique_write import DuplicateSettingError
 from settings.constants import ERR_SETTING_EXISTS
 from settings.contracts.schemas import SettingCreate, SettingScope
@@ -31,4 +32,6 @@ async def create_from_form(
         await service.create(data)
     except DuplicateSettingError:
         return redirect_back_with_errors(request, {"key": ERR_SETTING_EXISTS})
+    except ModuleKeyError as exc:
+        return redirect_back_with_errors(request, {"key": str(exc)})
     return RedirectResponse(redirect_to, status_code=303)