[rawo]

Closes #137. Part of the 2.0.0 breaking-change set.

Independent of the stacked schema PRs (#144 → #145 → #146), so it is based on main. It adds the same "From 1.x to 2.0.0" heading to UPGRADING.md as #144, which will be a trivial merge conflict for whichever lands second.

Problem

OutboxProcessor.processNext called listener.onEntryProcessed / onBatchProcessed right after store.updateAfterProcessingBatch, inside the scheduler's transaction, before it commits. When the commit then failed (connection loss, serialization failure, timeout):

• The rows rolled back to PENDING.
• okapi.entries.delivered and okapi.entries.failed had already been incremented, and they were incremented again on redelivery.
• A custom listener could act on state that was never persisted.

Fix

• OutboxProcessor.processBatch(limit) claims, delivers and persists, then returns a ProcessedBatch without notifying anyone.
• OutboxProcessor.publishEvents(batch) emits the entry events and then the batch event.
• OutboxScheduler runs processBatch inside runInTransaction and publishEvents after it returns. Under the scheduler, events therefore describe committed state, and a batch whose commit fails emits nothing.
• processNext(limit) keeps its signature and still notifies at once (processBatch + publishEvents), for callers that drive the processor themselves. Its KDoc says that inside a transaction those events come before the commit.
• onBatchProcessed's duration, and therefore okapi.batch.duration, now runs until the commit.
• Docs: the KDoc of OutboxProcessor, OutboxProcessorListener and OutboxScheduler; the README metrics table and listener paragraph; a CHANGELOG entry under Changed (BREAKING); an UPGRADING section.

Breaking

• Listener timing: under the scheduler, callbacks run after the commit, outside the processing transaction, and are skipped when the commit fails. A listener that relied on joining that transaction can't any more.
• Batch timer: okapi.batch.duration includes the commit.

Tests

• ListenerAfterCommitTest (unit):
  • processBatch doesn't notify, and publishEvents does;
  • processNext still notifies at once;
  • the scheduler emits events only after the runner's "commit";
  • a runner whose commit throws leads to no events over several ticks.
• ListenerAfterCommitEndToEndTest (Postgres): the listener queries the row's status from a separate connection inside onEntryProcessed and must see DELIVERED, which proves the commit already happened.
  • The test was checked against the fix: with the old scheduler it fails with expected:<DELIVERED> but was:<PENDING>.
• ./gradlew ktlintCheck test passes locally.

🤖 Generated with Claude Code

[rawo]

Looks right to me. I traced every shipped TransactionRunner on the scheduler's own threads and confirmed that "after runInTransaction returns" really does mean "after commit". I ran the new tests locally: ListenerAfterCommitTest (4/4) and ListenerAfterCommitEndToEndTest (1/1) pass. The comments below are mostly about documenting the contract this now relies on, and a few test and doc gaps.

What I checked

• Spring: SpringTransactionRunner uses TransactionTemplate.execute. On the outbox-processor thread and the worker threads there is no ambient transaction, so REQUIRED, REQUIRES_NEW and NESTED all start a new transaction and commit before execute returns. SUPPORTS, NOT_SUPPORTED and NEVER run in auto-commit, so writes are durable before return too. If a participant marks the transaction global rollback-only, the commit throws UnexpectedRollbackException, so publishEvents is skipped. A silent rollback only happens with local setRollbackOnly(), which okapi never calls.
• Exposed 1.6: transaction(db) with no outer transaction calls inTopLevelTransaction, which commits inside the loop before it returns. It retries the whole block on any SQLException, commit failures included, up to defaultMaxAttempts = 3. Each attempt makes a fresh ProcessedBatch. Only the attempt that commits is returned and published. That is correct, and better than before, when a failed attempt's events had already been emitted. With an outer Exposed transaction and useNestedTransactions = false there is no commit at return. The scheduler never hits that case, but a hand-rolled caller could (see the inline comment on OutboxScheduler.kt).
• Concurrency > 1: each worker runs processBatch() → publishEvents on its own thread, and ProcessedBatch is not shared. Nothing new is needed for thread safety beyond the existing rule that listeners must be thread-safe. Memory is the same as before: entries were already alive until notification.
• Exception paths: if processBatch throws (claim, delivery or updateAfterProcessingBatch), no events fire, which matches the old behaviour. Listener exceptions are still swallowed per callback inside publishEvents. If the commit fails, the scheduler logs the tick error and emits nothing.

Cross-cutting (no line in the diff to attach these to)

1. should-fix (docs): the counters now mean committed status transitions, not transport sends. When an attempt is rolled back, its messages still went out to Kafka/HTTP. That happens on a failed commit, and up to 3 times per tick with Exposed's retry loop. Those sends are never counted, and the redelivery is counted once. I think that's the right semantics, but the README metrics table / CHANGELOG should say so, because "okapi.entries.delivered: Successfully delivered entries" will be read as sends.
2. question: a failed commit now leaves no trace in metrics at all: no batch timer sample and no counter, only an ERROR log. Before, it was over-counted, which was wrong, but at least visible. Should there be a follow-up (e.g. onBatchFailed or an okapi.batch.failed counter) so operators can alert on commit failures? Not blocking for this PR.
3. nit (stale refs): CompositeMessageDeliverer.kt:62 still says the scheduler wraps "the whole OutboxProcessor.processNext cycle". It is processBatch now. The test name OutboxSchedulerTest "tick calls processNext with configured batchSize" is stale too.

[rawo]

should-fix (contract docs): the after-commit guarantee now rests on an unstated TransactionRunner contract: runInTransaction must have committed by the time it returns (it must not join an ambient transaction or defer the commit). The shipped runners meet it on the scheduler's threads (see the review body). But TransactionRunner's KDoc advertises "Standalone: user-provided lambda", and the Spring autoconfig already warns about ticks "joining an outer TX". A custom runner that participates in an outer transaction (Spring REQUIRED under an outer @Transactional, or Exposed transaction {} nested with useNestedTransactions = false) would silently bring back #137. Please spell out the requirement on TransactionRunner (and maybe in ExposedTransactionRunner/SpringTransactionRunner). It would also help to note that Exposed may retry the block, in which case only the attempt that commits is reported.

[rawo]

nit: the batch duration is read after all onEntryProcessed callbacks have run, so a slow listener (e.g. a composite doing I/O) inflates okapi.batch.duration. That doesn't match the README's new "from the claim until the transaction has committed". The old code had the same ordering, but now that the semantics are being redefined it's cheap to fix: take val endedAt = clock.instant() as the first statement of publishEvents and use it here.

[rawo]

nit: "Call it at most once per batch" is only documented. Calling it twice double-counts every Micrometer counter, which is exactly the class of bug this PR fixes. A one-shot guard is cheap: an AtomicBoolean in ProcessedBatch, with a second call either logged and ignored or failing via check. It would make the new public API harder to misuse.

[rawo]

nit (Java interop / API surface): internal doesn't hide these from Java. javap on the built class shows a plain public ProcessedBatch(List, Instant, Duration) constructor (Kotlin does not mangle internal constructors) and public getters getStartedAt$okapi_okapi_core() / getDeliveryDuration$okapi_okapi_core(). So Java callers can construct one, and those mangled names become de facto ABI. If the goal is "opaque token", a public class with a private constructor and a factory internal to the module (or an interface with an internal implementation) is tighter. Smaller points: no toString, so it logs as ProcessedBatch@1a2b; and entries exposes whatever list OutboxEntryProcessor returned (Java can cast and mutate it). Neither matters much, but this is new public API in a major release, so it's worth deciding now.

[rawo]

should-fix (test proves less than claimed): this runner is hand-rolled (db.jdbc.withTransaction), so the end-to-end test never exercises the runners users actually get: SpringTransactionRunner (via TransactionTemplate + DataSourceTransactionManager) and ExposedTransactionRunner. Both are already on this module's test classpath. Parametrising the test over those two (plus this one) would back the PR's claim that the fix holds "under the scheduler / Spring Boot autoconfiguration" with evidence, not just reasoning.

[rawo]

nit: if the row isn't found, rs.next() returns false, getString throws, and notifyEntry swallows the exception as a WARN. The future then never completes, and the test fails after 10 s with a bare TimeoutException instead of a useful message. Wrapping the body in try { ... } catch (e: Throwable) { statusSeenByListener.completeExceptionally(e) } (or completing with null when !rs.next()) makes failures diagnosable.

[rawo]

nit (coverage): a cheap extra case would pin down the Exposed retry behaviour: a runner that calls block() twice, where the first "commit" throws and the second succeeds, like inTopLevelTransaction on an SQLException. Assert that exactly one entry:Delivered / batch:1 pair is emitted. Another case could cover processBatch itself throwing (e.g. updateAfterProcessingBatch fails), asserting no events. Both behaviours are correct today but untested. The latch/timing in these tests looks sound, by the way: every log append happens before its countDown, and stop() waits for the in-flight tick.

[rawo]

nit: this holds only under the scheduler. With processNext inside the caller's transaction, the duration ends before the commit. It also includes the onEntryProcessed callbacks (see the comment on OutboxProcessor.kt:98). Maybe: "…from the claim until the scheduler's transaction has committed (processNext: until the results are written)".

[rawo]

nit: consider naming the most likely way this breaks for Ktor users explicitly. A listener that used the Exposed DSL with no transaction {} of its own, relying on the scheduler's thread-bound transaction, now gets IllegalStateException: No transaction in context on every event. OutboxProcessor swallows that as a WARN, so the listener silently stops working. In Spring, a listener calling a @Transactional(propagation = MANDATORY) method fails in the same silent way.

[rawo]

Addressed the review in 3bca3eb:

1. Undocumented commit contract. The TransactionRunner KDoc now states it: when runInTransaction returns normally, the transaction has committed. It also covers the pitfalls you listed (Spring REQUIRED inside @Transactional, nested Exposed transactions) and that a runner may retry the block, as Exposed does. UPGRADING repeats it for custom runners.
2. End-to-end test didn't use the real runners. ListenerAfterCommitEndToEndTest now runs the same tick three ways: a plain JDBC runner, SpringTransactionRunner (TransactionTemplate + DataSourceTransactionManager, with SpringConnectionProvider), and ExposedTransactionRunner (with ExposedConnectionProvider). All three see DELIVERED from a separate connection.
3. Counter semantics. Documented in the README, the CHANGELOG and UPGRADING: counters count committed status changes, not delivery attempts.
4. No metric for a failed commit. Agreed this is worth having, but it's additive (an onBatchFailed callback or an okapi.batch.failed counter can land in a minor release). I'll open a follow-up issue rather than grow this breaking PR.
5. Stale references. The CompositeMessageDeliverer KDoc now points at processBatch, and the scheduler test was renamed.
6. Slow listener inflated the batch timer. Fixed: the batch end time is read before the callbacks run.
7. Double publish. Fixed: publishEvents is now one-shot per batch, guarded by an AtomicBoolean, and a second call throws IllegalStateException. Tested.
8. ProcessedBatch from Java. entries is now an unmodifiable copy and the class has a toString. The internal timing fields stay internal-mangled. Java can technically call the constructor, but the KDoc says only OutboxProcessor creates batches, and a hand-built batch has no effect beyond publishing its own events.
9. Test timed out instead of failing. Fixed: the listener calls completeExceptionally, and a missing row fails with a message.
10. Missing unit tests. Added: a runner that runs the block twice (rolled back, then committed) produces exactly one set of events, after the commit; a processBatch that throws publishes nothing.
11. README batch timer wording. Qualified to cover only the scheduler and to exclude listener callbacks.
12. Silent failure on upgrade. UPGRADING now names it: Exposed's "No transaction in context" and Spring's MANDATORY failures, which are only logged as WARN.

./gradlew ktlintCheck test passes.