From 3b39fcd3111bcbdfa3b8f31deada3b421ba47e0b Mon Sep 17 00:00:00 2001 From: Bedram Tamang Date: Sat, 31 Oct 2026 13:26:12 -0700 Subject: [PATCH 1/2] Log after-commit callback failures and keep running the rest A raising after_commit callback propagated after the durable commit, skipped later callbacks and made the committed write look failed. Each failure is now logged with its traceback on the fastapi_startkit.masoniteorm logger and passed to an optional reporter set with DB.report_after_commit_failures_using. See ADR 057. Co-Authored-By: Claude Opus 5.5 --- .../adr/057-after-commit-callback-failures.md | 41 +++++ docs/adr/AGENTS.md | 1 + .../masoniteorm/connections/transactions.py | 33 +++- .../masoniteorm/facades/DB.py | 7 + .../connections/test_after_commit.py | 155 ++++++++++++++++-- 5 files changed, 220 insertions(+), 17 deletions(-) create mode 100644 docs/adr/057-after-commit-callback-failures.md 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..0e86ae33 --- /dev/null +++ b/docs/adr/057-after-commit-callback-failures.md @@ -0,0 +1,41 @@ +# 057 — After-commit callback failures are logged, not 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 error propagates out of `commit_transaction` (and out of `async with connection.transaction()`), the remaining callbacks never run, and the caller sees a failure for a write that is already committed. Applications such as Salut AI wrap every callback themselves to log the error, keep going and avoid reporting the committed write as failed. The framework should own that policy. + +## Alternatives + +1. **Keep propagating.** Rejected: a committed write looks failed, later callbacks are silently skipped, and every application repeats the same wrapper. +2. **Run every callback, then raise an aggregated `ExceptionGroup` behind an opt-in flag.** Rejected: whatever the flag, the error still surfaces at the commit site and reports a durable write as failed, and callers need `except*` around every transaction to tell callback failures from commit failures. Applications that want to be told about failures need a report, not an exception. +3. **Log and continue, with an optional reporter hook.** Every callback runs; a failing one is logged with its traceback and handed to an application-supplied reporter (for example Sentry). Chosen. + +## Decision + +- Each queued callback (sync, async or returning an awaitable) runs in order after the commit. An `Exception` raised by one is logged with `logger.exception` on the `fastapi_startkit.masoniteorm` logger, which the framework `LoggingHandler` on the root logger forwards to the configured log channel, and the next callback runs. The commit returns normally. +- `DB.report_after_commit_failures_using(handler)` (also importable from `fastapi_startkit.masoniteorm.connections.transactions`) registers one process-wide reporter `handler(error, callback)`, sync or async, called after the log entry for every failure. `None` removes it. A reporter that raises is itself logged and does not stop the remaining callbacks. +- Only `Exception` is handled. `BaseException`s such as `asyncio.CancelledError` and `KeyboardInterrupt` still propagate and stop the remaining callbacks, because swallowing cancellation would break task cancellation. +- Callbacks registered outside a transaction still run immediately; the same failure policy applies, since that write is already autocommitted. +- Unchanged: callbacks queued in a savepoint merge into the parent on release and run after the outermost commit, are discarded on rollback, and the connection is returned to the pool before any callback runs (`commit_transaction` releases it first), so a failing callback cannot leak a pooled connection. + +## Consequences + +- A callback failure no longer surfaces at the commit site. Code that relied on catching it there must use the reporter. +- The documentation's Database Transactions section, which says errors propagate after a durable commit, needs updating in the docs repository. + +## 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, for manual, context-manager and `Transaction.commit()` commits; +- the failure is logged on `fastapi_startkit.masoniteorm` at `ERROR` with its traceback; +- the row stays committed and a `pool_size=1, max_overflow=0` engine has no checked-out connection afterwards, and a following transaction works; +- a failing callback queued in a released savepoint runs, and fails without blocking the others, after the outer commit; +- an immediate (no transaction) failing callback is logged and does not raise; +- a sync and an async reporter receive the error and the callback; a raising reporter is logged and the remaining callbacks run; `None` removes the reporter; +- `CancelledError` from a callback still propagates. + +`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 86b6967c..89e7df30 100644 --- a/docs/adr/AGENTS.md +++ b/docs/adr/AGENTS.md @@ -46,3 +46,4 @@ Read this index first, then open the records relevant to the change. Before impl | [051](051-nullable-column-casts.md) | 2026-10-31 | Nullable annotations infer the cast of their inner type | `Caster.normalize_type` only knew bare types, so `int \| None`, `Optional[int]` and `Union[int, None]` fell through to the `"str"` cast and nullable columns (e.g. a `HasMany` foreign key) were read back as strings, breaking eager-load key matching. `Caster.build` (replacing `build_casts`) removes `None` from a top-level union, also around or inside `Annotated`, maps the inner type to its usual cast and records the field as nullable. For nullable fields only, `Caster.get` turns `None` into `None` (or the `Field` default) and `Caster.set` writes `None`; bare annotations keep their behaviour. Unions of several non-`None` types get a pass-through `BaseCast`. Relationship key matching stays type-strict. | | [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`. | +| [057](057-after-commit-callback-failures.md) | 2026-10-31 | After-commit callback failures are logged, not raised | A raising `after_commit` callback propagated after the durable commit, skipped the later callbacks and made the committed write look failed. Every callback now runs; an `Exception` is logged with its traceback on the `fastapi_startkit.masoniteorm` logger (forwarded to the framework log channel) and passed to an optional process-wide reporter set with `DB.report_after_commit_failures_using(handler)` (sync or async, `None` clears; a raising reporter is logged). The commit returns normally, the connection is released before callbacks run, and `BaseException`s such as `CancelledError` still propagate. An opt-in aggregated `ExceptionGroup` was rejected because it still reports a committed write as failed. 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..f9f00e00 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 @@ -10,14 +11,42 @@ AfterCommitCallback = Callable[[], object] +AfterCommitFailureReporter = Callable[[Exception, AfterCommitCallback], object] +logger = logging.getLogger("fastapi_startkit.masoniteorm") -async def _invoke(callback: AfterCommitCallback) -> None: - result = callback() +_failure_reporter: AfterCommitFailureReporter | None = None + + +def report_after_commit_failures_using(reporter: AfterCommitFailureReporter | None) -> None: + global _failure_reporter + _failure_reporter = reporter + + +async def _call(callable_: Callable[..., object], *args: object) -> None: + result = callable_(*args) if isawaitable(result): await result +async def _invoke(callback: AfterCommitCallback) -> None: + try: + await _call(callback) + except Exception as error: + logger.exception("After-commit callback %r failed", callback) + await _report(error, callback) + + +async def _report(error: Exception, callback: AfterCommitCallback) -> None: + reporter = _failure_reporter + if reporter is None: + return + try: + await _call(reporter, error, callback) + except Exception: + logger.exception("After-commit failure reporter %r failed", reporter) + + @dataclass(eq=False) class TransactionRecord: parent: TransactionRecord | None = None diff --git a/fastapi_startkit/src/fastapi_startkit/masoniteorm/facades/DB.py b/fastapi_startkit/src/fastapi_startkit/masoniteorm/facades/DB.py index 51a3e0bd..111e56bd 100644 --- a/fastapi_startkit/src/fastapi_startkit/masoniteorm/facades/DB.py +++ b/fastapi_startkit/src/fastapi_startkit/masoniteorm/facades/DB.py @@ -4,6 +4,7 @@ if TYPE_CHECKING: from fastapi_startkit.masoniteorm.connections.connection import AfterCommitCallback, Connection + from fastapi_startkit.masoniteorm.connections.transactions import AfterCommitFailureReporter from fastapi_startkit.masoniteorm.connections.manager import DatabaseManager from fastapi_startkit.masoniteorm.models.builder import QueryBuilder @@ -68,3 +69,9 @@ async def rollback(cls, name: str | None = None) -> None: @classmethod async def after_commit(cls, callback: AfterCommitCallback, name: str | None = None) -> None: await cls.instance().connection(name).after_commit(callback) + + @classmethod + def report_after_commit_failures_using(cls, reporter: "AfterCommitFailureReporter | None") -> None: + from fastapi_startkit.masoniteorm.connections.transactions import report_after_commit_failures_using + + report_after_commit_failures_using(reporter) diff --git a/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py b/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py index e3b72d6a..c35bf18d 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,157 @@ 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.fixture +def reporter(): + yield + DB.report_after_commit_failures_using(None) + + @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_is_logged_and_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() + with caplog.at_level(logging.ERROR, logger="fastapi_startkit.masoniteorm"): + await _commit(connection, mode, register) + + assert called == ["sync", "async"] + [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 "callback failed" in caplog.text 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 == ["sync", "async", "next"] + + +async def test_failing_sync_callback_does_not_stop_others(connection, caplog): + called = [] + + def fail(): + raise RuntimeError("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] + assert [type(record.exc_info[1]) for record in caplog.records] == [RuntimeError, ValueError] + + +async def test_failing_savepoint_callback_merges_and_runs_after_outer_commit(connection, caplog): + called = [] + 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 == [] + assert caplog.records == [] + await connection.after_commit(partial(called.append, "outer")) + assert called == ["nested", "outer"] + assert "nested failed" in caplog.text + assert connection.engine.pool.checkedout() == 0 assert await connection.select("SELECT id FROM records") == [{"id": 1}] + + +async def test_failing_immediate_callback_is_logged_without_raising(connection, caplog): + await connection.after_commit(_failing("immediate failed")) + assert "immediate failed" in caplog.text + + +@pytest.mark.parametrize("asynchronous", [False, True]) +async def test_reporter_receives_each_failure(connection, reporter, asynchronous): + reported = [] + + def sync_report(error, callback): + reported.append((str(error), callback)) + + async def async_report(error, callback): + await asyncio.sleep(0) + sync_report(error, callback) + + first = _failing("first") + second = _failing("second") + DB.report_after_commit_failures_using(async_report if asynchronous else sync_report) + async with connection.transaction(): + await connection.after_commit(first) + await connection.after_commit(lambda: None) + await connection.after_commit(second) + assert reported == [("first", first), ("second", second)] + + +async def test_raising_reporter_is_logged_and_callbacks_continue(connection, reporter, caplog): + called = [] + + def broken(error, callback): + raise RuntimeError("reporter failed") + + DB.report_after_commit_failures_using(broken) async with connection.transaction(): + await connection.after_commit(_failing("callback failed")) await connection.after_commit(partial(called.append, "next")) assert called == ["next"] + assert [type(record.exc_info[1]) for record in caplog.records] == [ValueError, RuntimeError] + assert "failure reporter" in caplog.records[1].getMessage() + + +async def test_clearing_reporter_stops_reporting(connection, reporter): + reported = [] + DB.report_after_commit_failures_using(lambda error, callback: reported.append(error)) + DB.report_after_commit_failures_using(None) + await connection.after_commit(_failing("unreported")) + assert reported == [] + + +async def test_cancelled_callback_still_propagates(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"]) From aff2f3412c98a3917b32e955d11e9199efeae118 Mon Sep 17 00:00:00 2001 From: Bedram Tamang Date: Sat, 31 Oct 2026 14:47:30 -0700 Subject: [PATCH 2/2] Raise after-commit callback failures after running every callback Drop the reporter hook and the log-and-continue policy. Every queued callback still runs; the first exception is re-raised unchanged once they finish, and later failures are logged. See ADR 057. Co-Authored-By: Claude Opus 5.5 --- .../adr/057-after-commit-callback-failures.md | 38 +++---- docs/adr/AGENTS.md | 2 +- .../masoniteorm/connections/transactions.py | 41 ++----- .../masoniteorm/facades/DB.py | 7 -- .../connections/test_after_commit.py | 106 +++++------------- 5 files changed, 60 insertions(+), 134 deletions(-) diff --git a/docs/adr/057-after-commit-callback-failures.md b/docs/adr/057-after-commit-callback-failures.md index 0e86ae33..688c2bd1 100644 --- a/docs/adr/057-after-commit-callback-failures.md +++ b/docs/adr/057-after-commit-callback-failures.md @@ -1,41 +1,41 @@ -# 057 — After-commit callback failures are logged, not raised +# 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 error propagates out of `commit_transaction` (and out of `async with connection.transaction()`), the remaining callbacks never run, and the caller sees a failure for a write that is already committed. Applications such as Salut AI wrap every callback themselves to log the error, keep going and avoid reporting the committed write as failed. The framework should own that policy. +`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 propagating.** Rejected: a committed write looks failed, later callbacks are silently skipped, and every application repeats the same wrapper. -2. **Run every callback, then raise an aggregated `ExceptionGroup` behind an opt-in flag.** Rejected: whatever the flag, the error still surfaces at the commit site and reports a durable write as failed, and callers need `except*` around every transaction to tell callback failures from commit failures. Applications that want to be told about failures need a report, not an exception. -3. **Log and continue, with an optional reporter hook.** Every callback runs; a failing one is logged with its traceback and handed to an application-supplied reporter (for example Sentry). Chosen. +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 -- Each queued callback (sync, async or returning an awaitable) runs in order after the commit. An `Exception` raised by one is logged with `logger.exception` on the `fastapi_startkit.masoniteorm` logger, which the framework `LoggingHandler` on the root logger forwards to the configured log channel, and the next callback runs. The commit returns normally. -- `DB.report_after_commit_failures_using(handler)` (also importable from `fastapi_startkit.masoniteorm.connections.transactions`) registers one process-wide reporter `handler(error, callback)`, sync or async, called after the log entry for every failure. `None` removes it. A reporter that raises is itself logged and does not stop the remaining callbacks. -- Only `Exception` is handled. `BaseException`s such as `asyncio.CancelledError` and `KeyboardInterrupt` still propagate and stop the remaining callbacks, because swallowing cancellation would break task cancellation. -- Callbacks registered outside a transaction still run immediately; the same failure policy applies, since that write is already autocommitted. -- Unchanged: callbacks queued in a savepoint merge into the parent on release and run after the outermost commit, are discarded on rollback, and the connection is returned to the pool before any callback runs (`commit_transaction` releases it first), so a failing callback cannot leak a pooled connection. +- `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 -- A callback failure no longer surfaces at the commit site. Code that relied on catching it there must use the reporter. -- The documentation's Database Transactions section, which says errors propagate after a durable commit, needs updating in the docs repository. +- 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, for manual, context-manager and `Transaction.commit()` commits; -- the failure is logged on `fastapi_startkit.masoniteorm` at `ERROR` with its traceback; -- the row stays committed and a `pool_size=1, max_overflow=0` engine has no checked-out connection afterwards, and a following transaction works; -- a failing callback queued in a released savepoint runs, and fails without blocking the others, after the outer commit; -- an immediate (no transaction) failing callback is logged and does not raise; -- a sync and an async reporter receive the error and the callback; a raising reporter is logged and the remaining callbacks run; `None` removes the reporter; -- `CancelledError` from a callback still propagates. +- 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 89e7df30..87a8af3b 100644 --- a/docs/adr/AGENTS.md +++ b/docs/adr/AGENTS.md @@ -46,4 +46,4 @@ Read this index first, then open the records relevant to the change. Before impl | [051](051-nullable-column-casts.md) | 2026-10-31 | Nullable annotations infer the cast of their inner type | `Caster.normalize_type` only knew bare types, so `int \| None`, `Optional[int]` and `Union[int, None]` fell through to the `"str"` cast and nullable columns (e.g. a `HasMany` foreign key) were read back as strings, breaking eager-load key matching. `Caster.build` (replacing `build_casts`) removes `None` from a top-level union, also around or inside `Annotated`, maps the inner type to its usual cast and records the field as nullable. For nullable fields only, `Caster.get` turns `None` into `None` (or the `Field` default) and `Caster.set` writes `None`; bare annotations keep their behaviour. Unions of several non-`None` types get a pass-through `BaseCast`. Relationship key matching stays type-strict. | | [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`. | -| [057](057-after-commit-callback-failures.md) | 2026-10-31 | After-commit callback failures are logged, not raised | A raising `after_commit` callback propagated after the durable commit, skipped the later callbacks and made the committed write look failed. Every callback now runs; an `Exception` is logged with its traceback on the `fastapi_startkit.masoniteorm` logger (forwarded to the framework log channel) and passed to an optional process-wide reporter set with `DB.report_after_commit_failures_using(handler)` (sync or async, `None` clears; a raising reporter is logged). The commit returns normally, the connection is released before callbacks run, and `BaseException`s such as `CancelledError` still propagate. An opt-in aggregated `ExceptionGroup` was rejected because it still reports a committed write as failed. Savepoint merge, rollback discard and immediate run outside a transaction are unchanged. | +| [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 f9f00e00..c315a69e 100644 --- a/fastapi_startkit/src/fastapi_startkit/masoniteorm/connections/transactions.py +++ b/fastapi_startkit/src/fastapi_startkit/masoniteorm/connections/transactions.py @@ -11,42 +11,16 @@ AfterCommitCallback = Callable[[], object] -AfterCommitFailureReporter = Callable[[Exception, AfterCommitCallback], object] logger = logging.getLogger("fastapi_startkit.masoniteorm") -_failure_reporter: AfterCommitFailureReporter | None = None - -def report_after_commit_failures_using(reporter: AfterCommitFailureReporter | None) -> None: - global _failure_reporter - _failure_reporter = reporter - - -async def _call(callable_: Callable[..., object], *args: object) -> None: - result = callable_(*args) +async def _invoke(callback: AfterCommitCallback) -> None: + result = callback() if isawaitable(result): await result -async def _invoke(callback: AfterCommitCallback) -> None: - try: - await _call(callback) - except Exception as error: - logger.exception("After-commit callback %r failed", callback) - await _report(error, callback) - - -async def _report(error: Exception, callback: AfterCommitCallback) -> None: - reporter = _failure_reporter - if reporter is None: - return - try: - await _call(reporter, error, callback) - except Exception: - logger.exception("After-commit failure reporter %r failed", reporter) - - @dataclass(eq=False) class TransactionRecord: parent: TransactionRecord | None = None @@ -64,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/src/fastapi_startkit/masoniteorm/facades/DB.py b/fastapi_startkit/src/fastapi_startkit/masoniteorm/facades/DB.py index 111e56bd..51a3e0bd 100644 --- a/fastapi_startkit/src/fastapi_startkit/masoniteorm/facades/DB.py +++ b/fastapi_startkit/src/fastapi_startkit/masoniteorm/facades/DB.py @@ -4,7 +4,6 @@ if TYPE_CHECKING: from fastapi_startkit.masoniteorm.connections.connection import AfterCommitCallback, Connection - from fastapi_startkit.masoniteorm.connections.transactions import AfterCommitFailureReporter from fastapi_startkit.masoniteorm.connections.manager import DatabaseManager from fastapi_startkit.masoniteorm.models.builder import QueryBuilder @@ -69,9 +68,3 @@ async def rollback(cls, name: str | None = None) -> None: @classmethod async def after_commit(cls, callback: AfterCommitCallback, name: str | None = None) -> None: await cls.instance().connection(name).after_commit(callback) - - @classmethod - def report_after_commit_failures_using(cls, reporter: "AfterCommitFailureReporter | None") -> None: - from fastapi_startkit.masoniteorm.connections.transactions import report_after_commit_failures_using - - report_after_commit_failures_using(reporter) diff --git a/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py b/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py index c35bf18d..4fdb19c5 100644 --- a/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py +++ b/fastapi_startkit/tests/masoniteorm/connections/test_after_commit.py @@ -189,14 +189,8 @@ async def fail(): return fail -@pytest.fixture -def reporter(): - yield - DB.report_after_commit_failures_using(None) - - @pytest.mark.parametrize("mode", ["manual", "context", "object"]) -async def test_failing_callback_is_logged_and_later_callbacks_run(connection, mode, caplog): +async def test_failing_callback_raises_after_later_callbacks_run(connection, mode, caplog): called = [] async def later(): @@ -209,16 +203,11 @@ async def register(): await connection.after_commit(partial(called.append, "sync")) await connection.after_commit(later) - with caplog.at_level(logging.ERROR, logger="fastapi_startkit.masoniteorm"): + with pytest.raises(ValueError, match="callback failed"): await _commit(connection, mode, register) assert called == ["sync", "async"] - [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 "callback failed" in caplog.text + assert caplog.records == [] assert connection.connection is None assert connection.engine.pool.checkedout() == 0 assert await connection.select("SELECT id FROM records") == [{"id": 1}] @@ -227,87 +216,48 @@ async def register(): assert called == ["sync", "async", "next"] -async def test_failing_sync_callback_does_not_stop_others(connection, caplog): +async def test_first_failure_raises_and_later_failures_are_logged(connection, caplog): called = [] def fail(): raise RuntimeError("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)) + 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] - assert [type(record.exc_info[1]) for record in caplog.records] == [RuntimeError, ValueError] + [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_merges_and_runs_after_outer_commit(connection, caplog): +async def test_failing_savepoint_callback_raises_after_outer_commit(connection): called = [] - async with connection.transaction(): - await connection.statement("INSERT INTO records VALUES (1)") + with pytest.raises(ValueError, match="nested failed"): async with connection.transaction(): - await connection.after_commit(_failing("nested failed")) - await connection.after_commit(partial(called.append, "nested")) - assert called == [] - assert caplog.records == [] - await connection.after_commit(partial(called.append, "outer")) + 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 "nested failed" in caplog.text assert connection.engine.pool.checkedout() == 0 assert await connection.select("SELECT id FROM records") == [{"id": 1}] -async def test_failing_immediate_callback_is_logged_without_raising(connection, caplog): - await connection.after_commit(_failing("immediate failed")) - assert "immediate failed" in caplog.text - - -@pytest.mark.parametrize("asynchronous", [False, True]) -async def test_reporter_receives_each_failure(connection, reporter, asynchronous): - reported = [] - - def sync_report(error, callback): - reported.append((str(error), callback)) - - async def async_report(error, callback): - await asyncio.sleep(0) - sync_report(error, callback) - - first = _failing("first") - second = _failing("second") - DB.report_after_commit_failures_using(async_report if asynchronous else sync_report) - async with connection.transaction(): - await connection.after_commit(first) - await connection.after_commit(lambda: None) - await connection.after_commit(second) - assert reported == [("first", first), ("second", second)] - - -async def test_raising_reporter_is_logged_and_callbacks_continue(connection, reporter, caplog): - called = [] - - def broken(error, callback): - raise RuntimeError("reporter failed") - - DB.report_after_commit_failures_using(broken) - async with connection.transaction(): - await connection.after_commit(_failing("callback failed")) - await connection.after_commit(partial(called.append, "next")) - assert called == ["next"] - assert [type(record.exc_info[1]) for record in caplog.records] == [ValueError, RuntimeError] - assert "failure reporter" in caplog.records[1].getMessage() - - -async def test_clearing_reporter_stops_reporting(connection, reporter): - reported = [] - DB.report_after_commit_failures_using(lambda error, callback: reported.append(error)) - DB.report_after_commit_failures_using(None) - await connection.after_commit(_failing("unreported")) - assert reported == [] +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_still_propagates(connection): +async def test_cancelled_callback_stops_remaining_callbacks(connection): called = [] async def cancel():