Skip to content

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

Closed
tmgbedu wants to merge 3 commits into
mainfrom
task/after-commit-callback-failures
Closed

tmgbedu wants to merge 3 commits into
mainfrom
task/after-commit-callback-failures

Conversation

@tmgbedu

@tmgbedu tmgbedu commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Before this change, a raising after_commit callback stopped the loop, so the remaining callbacks were skipped even though the write was already committed.
  • Now every queued callback runs (sync, async or returning an awaitable). After the last one, the first exception is re-raised unchanged with its original traceback. It surfaces from commit_transaction, Transaction.commit() or async with connection.transaction().
  • Any later failures are logged with their traceback on the fastapi_startkit.masoniteorm logger, so they aren't lost.
  • BaseExceptions such as CancelledError still propagate immediately.
  • The commit stays durable, and the connection goes back to the pool before callbacks run.
  • Not changed: outside a transaction, a callback runs immediately and raises directly. Callbacks still merge up from savepoints and are discarded on rollback.
  • There is no reporter hook and no log-and-continue mode. ADR 057 records why, and why an ExceptionGroup was rejected.

Docs follow-up

The Database Transactions section of docs/database/index.md in the docs repo says errors propagate after a durable commit. That is still true. It should add that the remaining callbacks still run and only the first error is raised.

Test plan

  • Updated tests in tests/masoniteorm/connections/test_after_commit.py:
    • first callback raises: the later sync and async callbacks still run, then the error is raised (manual, context and object commits);
    • the row stays committed and nothing is left checked out on a pool_size=1 engine;
    • with two failures, the first is raised and the second is logged;
    • a failing callback in a savepoint raises after the outer commit;
    • an immediate failing callback raises;
    • cancellation propagates.
  • uv run pytest --ignore=tests/masoniteorm/postgres --cov: 3722 passed, coverage 92.75%.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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 <noreply@anthropic.com>
@tmgbedu tmgbedu changed the title Log after-commit callback failures instead of raising after the commit Run every after-commit callback, then raise the first failure Oct 31, 2026
…lback-failures

# Conflicts:
#	docs/adr/AGENTS.md
@tmgbedu

tmgbedu commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Closing: stopping at the first failing after-commit callback is the intended behaviour and matches Laravel. DatabaseTransactionRecord::executeCallbacks() is a plain loop with no try/catch, so after the durable commit the first exception propagates to the caller and the remaining callbacks are skipped. Main already does this (covered by test_callback_error_propagates_after_commit_without_queue_leak). Callbacks that must not block the others should handle their own errors or dispatch a queued job.

@tmgbedu tmgbedu closed this Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant