Run every after-commit callback, then raise the first failure

docs/adr/057-after-commit-callback-failures.md

@@ -0,0 +1,41 @@
+# 057 — After-commit callbacks all run before a failure is raised
+
+- Date: 2026-10-31
+- Status: Accepted
+
+## Problem
+
+`DB.after_commit` / `Connection.after_commit` callbacks (ADR 006, ADR 018) run after the durable commit through `TransactionRecord.execute_callbacks`. When one callback raises, the loop stops, so the remaining callbacks never run even though the write they depend on is committed.
+
+## Alternatives
+
+1. **Keep stopping at the first failure.** Rejected: one broken callback silently skips every later one.
+2. **Log and continue without raising, with an optional reporter hook.** Rejected: a callback failure is an exception and the caller must see it. Swallowing it hides bugs and makes the framework choose an error policy that belongs to the application.
+3. **Raise an `ExceptionGroup` of every failure.** Rejected: callers would need `except*` around every transaction, and a single failure, by far the common case, would no longer be catchable as its own type.
+4. **Run every callback, then raise the first failure.** Chosen.
+
+## Decision
+
+- `execute_callbacks` runs every queued callback (sync, async or returning an awaitable) in order. The first `Exception` is kept, and the next callback runs. After the last callback, the first exception is re-raised unchanged, with its original traceback, from `commit_transaction`, `Transaction.commit()` or `async with connection.transaction()`.
+- A second or later failure is logged with `logger.exception` on the `fastapi_startkit.masoniteorm` logger (which the framework `LoggingHandler` on the root logger forwards to the log channel), so it is not lost. The raised exception is not logged, because the caller receives it.
+- Only `Exception` is collected. `BaseException`s such as `asyncio.CancelledError` and `KeyboardInterrupt` propagate at once and stop the remaining callbacks.
+- The commit is durable and the connection is returned to the pool before any callback runs (`commit_transaction` releases it first), so a raising callback cannot roll back the write or leak a pooled connection.
+- Unchanged: outside a transaction a callback runs immediately and its exception propagates directly; callbacks queued in a savepoint merge into the parent on release and run after the outermost commit; callbacks are discarded on rollback.
+
+## Consequences
+
+- An exception from the commit site may now come from a callback after a durable commit. Applications that must tell a failed commit from a failed callback should handle errors inside the callback.
+- The documentation's Database Transactions section ("errors propagate after a durable commit") stays accurate and should add that the remaining callbacks still run and only the first error is raised.
+
+## Validation
+
+`tests/masoniteorm/connections/test_after_commit.py`:
+
+- with the first of three callbacks raising, the second and third (one sync, one async) still run, then the first error is raised, for manual, context-manager and `Transaction.commit()` commits; nothing is logged;
+- the row stays committed, a `pool_size=1, max_overflow=0` engine has no checked-out connection afterwards, and a following transaction works;
+- with two failures, the first is raised and the second is logged on `fastapi_startkit.masoniteorm` at `ERROR` with its traceback;
+- a failing callback queued in a released savepoint runs after the outer commit, the others still run, and its error is raised;
+- an immediate (no transaction) failing callback raises;
+- `CancelledError` from a callback propagates and stops the remaining callbacks.
+
+`uv run pytest --ignore=tests/masoniteorm/postgres --cov` passes with coverage at or above 80%.

docs/adr/AGENTS.md

@@ -47,3 +47,4 @@ Read this index first, then open the records relevant to the change. Before impl
 | [052](052-sqlite-savepoint-isolation.md) | 2026-10-31 | SQLite savepoints start a real transaction | pysqlite/aiosqlite defer `BEGIN` until the first DML, so a savepoint opened first in an outer transaction became a top-level transaction and `RELEASE` committed it, leaking rows past `DatabaseTransaction`/`RefreshDatabase` rollbacks. `SQliteConnection` listens to the engine `savepoint` event and runs `BEGIN` when the driver connection is not in a transaction. On the shared `StaticPool` connection, a lost `BEGIN` race (`OperationalError` while a transaction is already open) is ignored, so concurrent nested transactions still succeed as on `main`. SQLAlchemy's `isolation_level=None` + `BEGIN` on every `begin` recipe was rejected because concurrent transactions on `SQLiteConfig`'s shared `StaticPool` connection then fail with `cannot start a transaction within a transaction`. |
 | [053](053-validated-reads-query-and-body.md) | 2026-10-31 | `Validated` reads the query string and the body | `Validated[Model]` declared a required `Body()`, so a `GET` with filters got `422 body: Field required` and apps bound models with `Query()`, skipping `authorize`, rules and `after`. The body parameter now defaults to `None`. Without a body, `GET`/`HEAD`/`OPTIONS`/`DELETE` validate `query_input`, while `POST`/`PUT`/`PATCH` still report `"": Field required` (per embed key when embedded), so a body-less cross-site form `POST` cannot pass on the query string (CSRF). `DELETE` always needs a CORS preflight. With a body, `request_input` merges `{**query, **body}` for an object; any other body, a JSON `null` included, passes through and fails with `model_type`. `query_input` keeps the keys pydantic accepts (`validation_alias`, `AliasChoices`, single-key `AliasPath`, the name with `validate_by_name`/`populate_by_name`). `collections.abc` `Sequence`/`Set` fields (not `str`/`bytes`) get every repeated value. Declared fields can be set from the URL on write methods with a JSON body, as with Laravel's `$request->all()`: keep sensitive fields off such models. `document_query_input` (installed by `FastAPIProvider`) replaces `requestBody` with `in: query` parameters on query-method routes. Chosen over a separate `ValidatedQuery` so one `FormRequest` serves `GET` and `POST`. |
 | [055](055-morph-name-equals-class-name.md) | 2026-10-31 | Morph relations accept the class name as the morph name | `MorphMany`/`MorphOne.get_record_key_lookup` raised `ValueError` whenever `Registry.get_morph_name` returned the class name, treating it as "not registered". `Model.__init_subclass__` registers every model with its class name as the default morph name, so every model without a `morph_map` alias (or mapped to its own class name) failed on `with_` and `load`. The lookup now returns `Registry.get_morph_name(cls)` as-is (`morph_map` aliases still win). `MorphMany`/`MorphOne.get_related` also forward `eagers` via `.with_(eagers or [])`, so nested loads such as `with_("media.lexeme")` work. `MorphTo`/`MorphToMany` were unaffected. |
+| [057](057-after-commit-callback-failures.md) | 2026-10-31 | After-commit callbacks all run before a failure is raised | A raising `after_commit` callback stopped the loop, so later callbacks were skipped after a durable commit. `TransactionRecord.execute_callbacks` now runs every callback, keeps the first `Exception` and re-raises it unchanged after the last one; later failures are logged with their traceback on the `fastapi_startkit.masoniteorm` logger. `BaseException`s such as `CancelledError` propagate at once. The connection is released before callbacks run. Log-and-continue with a reporter hook was rejected because a failure is an exception the caller must see; an `ExceptionGroup` was rejected because it forces `except*`. Savepoint merge, rollback discard and immediate run outside a transaction are unchanged. |

fastapi_startkit/src/fastapi_startkit/masoniteorm/connections/transactions.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import logging
 from collections.abc import Callable
 from dataclasses import dataclass, field
 from inspect import isawaitable
@@ -11,6 +12,8 @@
 
 AfterCommitCallback = Callable[[], object]
 
+logger = logging.getLogger("fastapi_startkit.masoniteorm")
+
 
 async def _invoke(callback: AfterCommitCallback) -> None:
     result = callback()
@@ -35,8 +38,17 @@ def descends_from(self, record: TransactionRecord) -> bool:
         return False
 
     async def execute_callbacks(self) -> None:
+        first_error: Exception | None = None
         for callback in self.callbacks:
-            await _invoke(callback)
+            try:
+                await _invoke(callback)
+            except Exception as error:
+                if first_error is None:
+                    first_error = error
+                else:
+                    logger.exception("After-commit callback %r failed", callback)
+        if first_error is not None:
+            raise first_error
 
 
 class TransactionsManager:

fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py

@@ -1,5 +1,6 @@
 from fastapi_startkit.masoniteorm import SQLiteConfig
 import asyncio
+import logging
 from contextvars import Context
 from functools import partial
 from unittest.mock import patch
@@ -169,33 +170,107 @@ async def test_ancestor_commit_includes_open_savepoint_callbacks(connection, exp
     assert called == ["root", "child", "grandchild"]
 
 
+async def _commit(connection, mode, register):
+    if mode == "manual":
+        await connection.begin_transaction()
+        await register()
+        await connection.commit_transaction()
+    else:
+        async with connection.transaction() as transaction:
+            await register()
+            if mode == "object":
+                await transaction.commit()
+
+
+def _failing(message):
+    async def fail():
+        raise ValueError(message)
+
+    return fail
+
+
 @pytest.mark.parametrize("mode", ["manual", "context", "object"])
-async def test_callback_error_propagates_after_commit_without_queue_leak(connection, mode):
+async def test_failing_callback_raises_after_later_callbacks_run(connection, mode, caplog):
     called = []
 
-    async def fail():
-        raise ValueError("callback failed")
+    async def later():
+        await asyncio.sleep(0)
+        called.append("async")
 
     async def register():
         await connection.statement("INSERT INTO records VALUES (1)")
-        await connection.after_commit(fail)
-        await connection.after_commit(partial(called.append, "skip"))
+        await connection.after_commit(_failing("callback failed"))
+        await connection.after_commit(partial(called.append, "sync"))
+        await connection.after_commit(later)
 
     with pytest.raises(ValueError, match="callback failed"):
-        if mode == "manual":
-            await connection.begin_transaction()
-            await register()
-            await connection.commit_transaction()
-        else:
-            async with connection.transaction() as transaction:
-                await register()
-                if mode == "object":
-                    await transaction.commit()
+        await _commit(connection, mode, register)
+
+    assert called == ["sync", "async"]
+    assert caplog.records == []
     assert connection.connection is None
+    assert connection.engine.pool.checkedout() == 0
     assert await connection.select("SELECT id FROM records") == [{"id": 1}]
     async with connection.transaction():
         await connection.after_commit(partial(called.append, "next"))
-    assert called == ["next"]
+    assert called == ["sync", "async", "next"]
+
+
+async def test_first_failure_raises_and_later_failures_are_logged(connection, caplog):
+    called = []
+
+    def fail():
+        raise RuntimeError("sync failed")
+
+    with pytest.raises(RuntimeError, match="sync failed"):
+        async with connection.transaction():
+            await connection.after_commit(fail)
+            await connection.after_commit(partial(called.append, 2))
+            await connection.after_commit(_failing("async failed"))
+            await connection.after_commit(partial(called.append, 3))
+    assert called == [2, 3]
+    [record] = caplog.records
+    assert record.name == "fastapi_startkit.masoniteorm"
+    assert record.levelno == logging.ERROR
+    assert "After-commit callback" in record.getMessage()
+    assert isinstance(record.exc_info[1], ValueError)
+    assert "async failed" in caplog.text
+
+
+async def test_failing_savepoint_callback_raises_after_outer_commit(connection):
+    called = []
+    with pytest.raises(ValueError, match="nested failed"):
+        async with connection.transaction():
+            await connection.statement("INSERT INTO records VALUES (1)")
+            async with connection.transaction():
+                await connection.after_commit(_failing("nested failed"))
+                await connection.after_commit(partial(called.append, "nested"))
+            assert called == []
+            await connection.after_commit(partial(called.append, "outer"))
+    assert called == ["nested", "outer"]
+    assert connection.engine.pool.checkedout() == 0
+    assert await connection.select("SELECT id FROM records") == [{"id": 1}]
+
+
+async def test_failing_immediate_callback_raises(connection):
+    with pytest.raises(ValueError, match="immediate failed"):
+        await connection.after_commit(_failing("immediate failed"))
+
+
+async def test_cancelled_callback_stops_remaining_callbacks(connection):
+    called = []
+
+    async def cancel():
+        raise asyncio.CancelledError
+
+    with pytest.raises(asyncio.CancelledError):
+        async with connection.transaction():
+            await connection.statement("INSERT INTO records VALUES (1)")
+            await connection.after_commit(cancel)
+            await connection.after_commit(partial(called.append, "skipped"))
+    assert called == []
+    assert connection.engine.pool.checkedout() == 0
+    assert await connection.select("SELECT id FROM records") == [{"id": 1}]
 
 
 @pytest.mark.parametrize("mode", ["manual", "object"])