diff --git a/docs/adr/057-after-commit-callback-failures.md b/docs/adr/057-after-commit-callback-failures.md new file mode 100644 index 00000000..688c2bd1 --- /dev/null +++ b/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%. diff --git a/docs/adr/AGENTS.md b/docs/adr/AGENTS.md index 4952cfc2..9eafdf5a 100644 --- a/docs/adr/AGENTS.md +++ b/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. | diff --git a/fastapi_startkit/src/fastapi_startkit/masoniteorm/connections/transactions.py b/fastapi_startkit/src/fastapi_startkit/masoniteorm/connections/transactions.py index de0a7f85..c315a69e 100644 --- a/fastapi_startkit/src/fastapi_startkit/masoniteorm/connections/transactions.py +++ b/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: diff --git a/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py b/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py index e3b72d6a..4fdb19c5 100644 --- a/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py +++ b/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"])