Tech-debt batch 7: users durable checkpoints via independent_session, internal split; domain errors in users/settings (#592 #595)

CHANGELOG.md

@@ -19,6 +19,15 @@ All notable changes to this project are documented in this file. The format is b
   installed, otherwise run the durable handlers inline in `db`'s transaction (#583).
 
 ### Changed
+- Users: lockout, registration and bulk-invite writes that must land before the request ends
+  now commit through `simple_module_db.independent_session` (its own short-lived session)
+  instead of committing the request session. Failed or unverified password logins now roll
+  back the request session before the lockout bookkeeping is checkpointed. Lockout,
+  invite and demo code moved into internal subpackages (#592).
+- Users login lockout raises `LoginLockedError`, mapped to the same 429 body and `Retry-After`
+  by a registered exception handler; settings errors now share a `SettingsError` base (their
+  HTTP mapping is unchanged). The convention is in
+  `docs/module-authoring.md` and the module scaffold (#595).
 - Packaging: **Behaviour change.** Published packages now cap their third-party runtime
   dependencies (next major for 1.x+ libraries, next minor for 0.x ones such as `sqlmodel`), so
   an install can no longer resolve an untested major release (#600).

docs/database/sessions.md

@@ -112,8 +112,9 @@ At callback time the original transaction is already durable, so callback errors
 cannot roll it back. The framework logs a `db.session.on_commit_failed` error,
 continues with remaining callbacks, and preserves the successful response.
 
-`on_commit()` belongs to the framework-managed request lifecycle. For a manually
-managed session, perform post-commit work explicitly after the transaction block.
+`on_commit()` is supported by framework-managed requests and independent
+checkpoints. For other manually managed sessions, perform post-commit work
+explicitly after the transaction block.
 
 ## Manual transactions
 
@@ -133,20 +134,41 @@ async def worker(db: DatabaseState):
 
 The `async with session.begin()` pattern opens a transaction you control. Use it in code that runs **outside** a request scope.
 
-### `independent_session(app)`
+### `independent_session(source)`
 
-For the common case, a short unit of work outside the request that should commit when it succeeds and roll back when it fails, use `independent_session`:
+Use an independent checkpoint when a write must be durable before continuing,
+including before a request can fail. Examples are reserving a login attempt
+before checking its password and saving invitation roles before sending mail.
+Ordinary request writes still belong to `CommitBeforeResponseMiddleware`.
 
 ```python
 from simple_module_db import independent_session
 
 
-async def on_startup(self, app):
+async def reserve_attempt(app):
     async with independent_session(app) as db:
-        await replay_rows(db)  # committed on clean exit, rolled back on error
+        await reserve_slot(db)
+    # The slot is durable here, even if subsequent authentication fails.
 ```
 
-It opens a session from `app.state.sm.db.session_factory`, commits when the block exits cleanly, and rolls back and re-raises otherwise. Do not call `commit()` inside the block. Keep using `session_factory()` directly when the block is read-only, must commit mid-way (for example to do follow-up work after the commit, as the branding image reaper does), or runs against another engine.
+The source can be an app, its `app.state.sm.db.session_factory`, or a
+framework-created session (which supplies that same factory). The helper opens
+and closes its own short-lived session, commits on clean exit, and rolls back
+and re-raises on error. It never commits or transfers the source session's
+pending work. Do not call `commit()` inside the block.
+
+On SQLite, an independent session cannot write while the request session holds uncommitted writes (there is only one writer), so call it before the request writes, as users lockout and bulk invite do.
+
+`on_commit` callbacks run in registration order after the checkpoint commits;
+rollback or commit failure discards them. Callback failures are logged without
+undoing the durable checkpoint, as for request sessions.
+
+A checkpoint cannot see uncommitted request rows. Pass identifiers and reload
+objects in the independent session rather than sharing attached ORM objects.
+Keep checkpoints short and open them before the request holds a write
+transaction: SQLite permits only one writer, and overlapping writes can block.
+Use `session_factory()` directly for read-only work or transactions needing
+several explicit commit boundaries.
 
 ## Sessions and `MultiTenantMixin`
 

docs/framework/request-guards.md

@@ -166,7 +166,7 @@ which counts, a store has `peek(key, spec)`, which reports the current window
 without counting (`allowed` answers "would one more hit be within budget"),
 `reset(key)`, which forgets a window, and `hit(..., sliding=True)`, which
 restarts the window on every hit. The users login lockout
-(`users/auth_local/lockout.py`) uses all three. Keys are
+(`users/auth_local/lockout/`) uses all three. Keys are
 namespaced by the caller (`login:…`); Redis keys get the `sm:rl:` prefix.
 
 ### Response shape

docs/module-authoring.md

@@ -85,6 +85,20 @@ canonical example is `file_storage.StorageBackend`: it has a registry, two
 shipped implementations (`FilesystemBackend`, `S3Backend`), and tests that
 mock against the Protocol. If none of those apply to your module, skip it.
 
+### Service errors: domain exceptions
+
+Services raise domain exceptions derived from a per-module base error in
+`<module>/exceptions.py` or `errors.py`; they must not raise `HTTPException`.
+Keep the failure and its context independent of HTTP so CLI commands, tasks
+and durable handlers can reuse the same business logic.
+
+Follow `file_storage`: implement the HTTP mapping in the endpoint layer or
+install handlers from `register_exception_handlers(app)`. Preserve each
+endpoint's status, JSON body, headers and translation keys when migrating an
+existing service. Translate messages at the request boundary. Endpoint-level
+`HTTPException` remains appropriate for pure request validation. Inertia forms
+may catch domain errors to report field errors or redirect instead.
+
 ### Minimal `pyproject.toml`
 
 ```toml

framework/cli/simple_module_cli/templates/module/__PACKAGE__/service.py.tpl

@@ -1,4 +1,10 @@
-"""{{SINGULAR_CLASS}} service implementation."""
+"""{{SINGULAR_CLASS}} service implementation.
+
+Business failures raise domain exceptions derived from a per-module base in
+exceptions.py or errors.py, never HTTPException. Map them in endpoints or via
+ModuleBase.register_exception_handlers; keep HTTP status, payload, headers and
+translation at that boundary so this service also works in CLI/tasks.
+"""
 
 from __future__ import annotations
 

framework/db/simple_module_db/independent.py

@@ -1,34 +1,52 @@
-"""A session opened outside the request, with the commit convention in one place."""
+"""Short-lived durable transactions, independent of the request unit of work."""
 
 from __future__ import annotations
 
 from collections.abc import AsyncGenerator
 from contextlib import asynccontextmanager
 from typing import Any
 
-from sqlalchemy.ext.asyncio import AsyncSession
+from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker
+
+from simple_module_db.callbacks import discard_on_commit_callbacks, run_on_commit_callbacks
+from simple_module_db.session import SESSION_FACTORY_KEY, RequestSession
 
 
 @asynccontextmanager
-async def independent_session(app: Any) -> AsyncGenerator[AsyncSession]:
-    """Open a session from ``app``'s factory, outside any request.
+async def independent_session(source: Any) -> AsyncGenerator[RequestSession]:
+    """Commit an independent session on success; roll back and re-raise on error.
 
-    Commits when the block exits cleanly and rolls back (then re-raises) when it
-    raises, so startup hooks, background cleanups and out-of-band writes do not
-    each hand-roll the pair. The session is closed either way.
+    Accept an app or session factory. An existing session can also supply its
+    framework session factory; its pending work is never transferred.
+    Request code may use this for a deliberate durable checkpoint, whose writes
+    must survive a later request failure. The new session cannot see pending
+    request writes. Keep checkpoints short and avoid holding a write transaction
+    on the same database while opening one (especially on SQLite).
 
-    Request code must not use this: the request session (``get_db``) commits in
-    ``CommitBeforeResponseMiddleware``, and a second session cannot see its
-    uncommitted rows. Code that must commit mid-block, never commit, or run on a
-    different engine keeps using ``session_factory()`` directly.
+    Post-commit callbacks run in registration order after the commit succeeds.
+    The session closes on every exit, including cancellation.
     """
-    async with app.state.sm.db.session_factory() as session:
+    if isinstance(source, async_sessionmaker):
+        factory = source
+    elif isinstance(source, AsyncSession):
+        try:
+            factory = source.info[SESSION_FACTORY_KEY]
+        except KeyError:
+            raise TypeError(
+                "independent_session(session) needs a session opened by the framework's "
+                "session factory; pass the app or the async_sessionmaker instead"
+            ) from None
+    else:
+        factory = source.state.sm.db.session_factory
+    async with factory() as session:
         try:
             yield session
             await session.commit()
-        except Exception:
-            await session.rollback()
+        except BaseException as exc:
+            discard_on_commit_callbacks(session)
+            # Closing the session handles cancellation without masking it with
+            # another cancellable await; ordinary failures roll back explicitly.
+            if isinstance(exc, Exception):
+                await session.rollback()
             raise
-        # Cancellation and other BaseExceptions skip the explicit rollback: awaiting
-        # it there could be cancelled too and mask the original error, and closing
-        # the session discards the open transaction anyway.
+        await run_on_commit_callbacks(session)

framework/db/simple_module_db/session.py

@@ -19,6 +19,8 @@
 from simple_module_db.provider import DatabaseProvider, detect_provider
 from simple_module_db.tenancy import DEFAULT_TENANT_ID
 
+SESSION_FACTORY_KEY = "sm_session_factory"
+
 
 class RequestSession(AsyncSession):
     """Async session with hooks for the request-owned transaction."""
@@ -135,6 +137,8 @@ def init_db(
         sync_session_class=scoped_session_class,
     )
 
+    session_factory.configure(info={SESSION_FACTORY_KEY: session_factory})
+
     return DatabaseState(
         engine=engine,
         session_factory=session_factory,

framework/db/simple_module_db/transaction.py

@@ -130,11 +130,13 @@ async def finalize_session(session: AsyncSession) -> None:
 async def rollback_session(session: AsyncSession) -> None:
     """Roll ``session`` back and drop its pending work. Idempotent."""
     discard_on_commit_callbacks(session)
-    if session.info.get(_FINALIZED_KEY) and not _has_pending(session):
+    has_pending = _has_pending(session)
+    if session.info.get(_FINALIZED_KEY) and not has_pending:
         return
     _settle(session)
     await session.rollback()
-    logger.warning(
+    logger.log(
+        logging.WARNING if has_pending else logging.DEBUG,
         "db.session.rollback",
         extra={"operation": "rollback", "db_duration_ms": _elapsed_ms(session)},
     )

framework/db/tests/test_db_independent_checkpoint.py

@@ -0,0 +1,72 @@
+"""Independent checkpoints own their transaction and post-commit callbacks."""
+
+from types import SimpleNamespace
+
+import pytest
+from simple_module_db import independent_session
+from sqlalchemy import text
+
+
+@pytest.fixture
+async def checkpoint_db(db_state):
+    async with db_state.engine.begin() as connection:
+        await connection.execute(text("CREATE TABLE checkpoint_row (id integer primary key)"))
+    yield db_state
+    async with db_state.engine.begin() as connection:
+        await connection.execute(text("DROP TABLE checkpoint_row"))
+
+
+async def test_factory_checkpoint_runs_callbacks_after_durable_commit(checkpoint_db):
+    observed = []
+
+    async def observe():
+        async with checkpoint_db.session_factory() as reader:
+            observed.append((await reader.execute(text("SELECT id FROM checkpoint_row"))).scalar())
+
+    async with independent_session(checkpoint_db.session_factory) as session:
+        await session.execute(text("INSERT INTO checkpoint_row VALUES (1)"))
+        session.on_commit(observe)
+        session.on_commit(lambda: observed.append("second"))
+    assert observed == [1, "second"]
+
+
+async def test_app_checkpoint_discards_callbacks_on_failure(checkpoint_db):
+    app = SimpleNamespace(state=SimpleNamespace(sm=SimpleNamespace(db=checkpoint_db)))
+    observed = []
+    with pytest.raises(RuntimeError, match="abort"):
+        async with independent_session(app) as session:
+            await session.execute(text("INSERT INTO checkpoint_row VALUES (1)"))
+            session.on_commit(lambda: observed.append("committed"))
+            raise RuntimeError("abort")
+    assert observed == []
+    async with checkpoint_db.session_factory() as reader:
+        assert (await reader.execute(text("SELECT count(*) FROM checkpoint_row"))).scalar() == 0
+
+
+async def test_session_checkpoint_leaves_request_work_and_callbacks_uncommitted(tmp_path):
+    from simple_module_db import init_db
+    from sqlmodel import Field, SQLModel
+
+    class CheckpointRow(SQLModel, table=True):
+        __tablename__ = "db_checkpoint_isolation"
+        id: int = Field(primary_key=True)
+
+    state = init_db(f"sqlite+aiosqlite:///{tmp_path / 'checkpoint.db'}")
+    observed = []
+    try:
+        async with state.engine.begin() as connection:
+            await connection.run_sync(CheckpointRow.__table__.create)
+        async with state.session_factory() as request:
+            request.add(CheckpointRow(id=1))
+            request.on_commit(lambda: observed.append("request"))
+            async with independent_session(request) as checkpoint:
+                assert checkpoint is not request
+                checkpoint.add(CheckpointRow(id=2))
+                checkpoint.on_commit(lambda: observed.append("checkpoint"))
+            assert observed == ["checkpoint"]
+            await request.rollback()
+        async with state.session_factory() as reader:
+            rows = (await reader.execute(text("SELECT id FROM db_checkpoint_isolation"))).scalars()
+            assert list(rows) == [2]
+    finally:
+        await state.engine.dispose()

framework/db/tests/test_rollback_logging.py

@@ -0,0 +1,23 @@
+"""Only rollbacks that discard writes warrant a warning."""
+
+import logging
+
+import pytest
+from _models import _TxnThing
+from simple_module_db import mark_written
+from simple_module_db.transaction import rollback_session
+from sqlalchemy.ext.asyncio import AsyncSession
+
+
+@pytest.mark.parametrize("pending", ["none", "new", "written"])
+async def test_rollback_log_level_matches_pending_work(caplog, pending):
+    async with AsyncSession() as session:
+        if pending == "new":
+            session.add(_TxnThing(name="discarded"))
+        elif pending == "written":
+            mark_written(session)
+        with caplog.at_level(logging.DEBUG, logger="simple_module.db"):
+            await rollback_session(session)
+    records = [record for record in caplog.records if record.message == "db.session.rollback"]
+    assert len(records) == 1
+    assert records[0].levelno == (logging.DEBUG if pending == "none" else logging.WARNING)

modules/settings/settings/_managed_keys.py

@@ -25,11 +25,12 @@
 from settings.constants import ERR_MANAGED_KEY_DELETE
 from settings.contracts.registry import SettingsRegistry, clear_route
 from settings.contracts.schemas import SettingScope
+from settings.exceptions import SettingsError
 
 TenantIsLive = Callable[[str], Awaitable[bool]]
 
 
-class ManagedKeyError(Exception):
+class ManagedKeyError(SettingsError):
     """The key is cleared through ``clear_via``, not by deleting its row."""
 
     def __init__(self, key: str, clear_via: str) -> None:

modules/settings/settings/_unique_write.py

@@ -12,8 +12,10 @@
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.ext.asyncio import AsyncSession
 
+from settings.exceptions import SettingsError
 
-class DuplicateSettingError(Exception):
+
+class DuplicateSettingError(SettingsError):
     """A row with this (scope, scope_id, key) already exists."""
 
 

modules/settings/settings/exceptions.py

@@ -0,0 +1,5 @@
+"""Transport-independent settings business failures."""
+
+
+class SettingsError(Exception):
+    """Base for errors raised by settings services."""

modules/settings/tests/test_settings_domain_errors.py

@@ -0,0 +1,10 @@
+"""Settings business failures share a transport-independent base."""
+
+
+def test_settings_errors_share_a_module_base():
+    from settings._managed_keys import ManagedKeyError
+    from settings._unique_write import DuplicateSettingError
+    from settings.exceptions import SettingsError
+
+    assert isinstance(ManagedKeyError("key", "/clear"), SettingsError)
+    assert isinstance(DuplicateSettingError("key"), SettingsError)

modules/users/tests/test_demo_guard.py → modules/users/tests/demo/test_users_demo_guard.py

@@ -9,7 +9,7 @@
 import httpx
 from users.constants import ADMIN_ROLE_NAME, SESSION_USER_ID_KEY, USER_ROLE_NAME
 from users.demo import SESSION_DEMO_KEY
-from users.demo_guard import DEMO_READ_ONLY_DETAIL
+from users.demo.guard import DEMO_READ_ONLY_DETAIL
 
 
 def _demo_route(role: str) -> str:

modules/users/tests/test_admin_resend_invite.py → modules/users/tests/invites/test_users_admin_resend_invite.py

@@ -202,7 +202,7 @@ class TestMailerFailureIsNotEchoed:
     async def test_the_admin_gets_the_link_and_a_generic_reason(
         self, admin_client, users_app, caplog
     ):
-        from users.admin.bulk_invite import MAILER_FAILURE_DETAIL
+        from users.invites.bulk import MAILER_FAILURE_DETAIL
 
         class _BrokenMailer(_SpyMailer):
             async def send_invite(self, email, token, invited_by_name, message=None):
@@ -212,7 +212,7 @@ async def send_invite(self, email, token, invited_by_name, message=None):
         original = users_app.state.users.mailer
         users_app.state.users.mailer = _BrokenMailer()
         try:
-            with caplog.at_level(logging.ERROR, logger="users.admin.resend_invite"):
+            with caplog.at_level(logging.ERROR, logger="users.invites.resend"):
                 resp = await admin_client.post(f"/api/users/admin/{user_id}/resend-invite")
         finally:
             users_app.state.users.mailer = original