feat(outbox): extract OutboxBackend storage SPI (Wave A.2 Phase 0a) #7
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/outbox-backend-spi"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Wave A.2 Phase 0a — OutboxBackend SPI extraction (zero behaviour change)
Introduces an
OutboxBackendinterface as the abstraction seam between the relay/poller orchestration (OutboxPublisher+OutboxPollerWorker) and the persistence engine. Zero behaviour change for the 7 PG consumers — this de-risks Phase 0b (the Redis Lua-EVAL backend) by landing the seam first, behind characterization tests. Design:.planning/32-wave-a2-redis-outbox-design.mdrefinement #11.Why
identity-service is Redis-only; its ticket mint+event atomicity cannot use the PG outbox (two stores, no shared transaction). A Redis-resident outbox is required, but must plug into the same publisher/poller relay machinery the PG services use — hence an explicit SPI, not a second parallel publisher (ADR-0014 D-3 /
docs/decisions/0014-supporting/identity-redis-outbox-backend.md).Surface
OutboxBackend—persistPending,findPendingBatch(int),markSent,markFailureAttempt(mirrorOutboxPublishCompletion),requiresActiveTransaction().PgOutboxBackend(default) — thin adapter wrappingOutboxRecordRepository(persist + FIFO batch fetch) +OutboxPublishCompletion(REQUIRES_NEW transitions, delegated verbatim to preserve the AFTER_COMMIT post-commit transaction boundary).requiresActiveTransaction() == true.OutboxPublisher/OutboxPollerWorker—repository+completioncollapse behind a singleOutboxBackend. Tx-guard extracted torequireActiveTransactionIfBackendDemands()gated onbackend.requiresActiveTransaction()(true for PG → precondition unchanged). AFTER_COMMIT relay, Resilience4j breaker, OTel spans, defensive clone: untouched.OutboxAutoConfiguration— new@Bean @ConditionalOnMissingBean outboxBackend=PgOutboxBackend.OutboxProperties— newim2be.outbox.backendenum{PG, REDIS}, defaultPG.REDISreserved/no-op until Phase 0b (explicit selector, not classpath presence).Behaviour-change verification
OutboxPublisherTest(27) +OutboxPublisherIT(4): every assertion byte-for-byte preserved; only construction sites changed (same mocks wrapped inPgOutboxBackend, which delegates 1:1).OutboxPollerWorkerTest(13) likewise.PgOutboxBackendTest(9, pins verbatim delegation + thetruetx flag),OutboxBackendTest(4, proves the publisher tx-guard is gated onrequiresActiveTransaction()— both arms — via an in-memory fake).mvn -pl outbox-publisher -am clean verify→ BUILD SUCCESS, 0[WARNING], 94 unit + 4 IT green.Note
markSent/markFailureAttemptare typed to return the applied rowcount butPgOutboxBackendreturns a fixed1:OutboxPublishCompletionreturns void and the orchestration layer never branches on the rowcount, so behaviour is identical to the pre-extraction path. A pre-existing[ERROR] Surefire ... kill self fork JVMdiagnostic on the Testcontainers IT (lingering OTel/Kafka thread) is unrelated to this DI-only refactor; BUILD SUCCESS confirms it is non-fatal.Show previous round
hib-pr-reviewer review — PR #7 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
eb800eee586e, basemain, triggeropenedTL;DR: CONDITIONAL_APPROVE — kept 4 minor findings (1 agreed A+B, 3 unique-to-one verified by code inspection); no blocking issues; SPI extraction itself is clean.
Summary
Arbitration — PR #7: OutboxBackend SPI extraction (Wave A.2 Phase 0a)
Verification run: Read
OutboxAutoConfiguration.javalines 204–284,PgOutboxBackend.javalines 100–145,OutboxBackendTest.javalines 95–117, andOutboxProperties.javalines 100–130.A1 (silent misconfiguration, unique-to-A) → KEPT. Lines 227–234 of
OutboxAutoConfiguration.javaconfirmoutboxBackend()accepts onlyOutboxRecordRepository+OutboxPublishCompletion;OutboxPropertiesis absent. An operator settingim2be.outbox.backend=REDISreceivesPgOutboxBackendsilently.OutboxProperties.Backend.REDISJavadoc (lines 112–119) does note "no effect," but only if the operator reads the enum source — no startup-log signal exists. Finding verified.A2/B1 (return-value contract, agreed) → KEPT as single finding. Both reviewers independently flagged
PgOutboxBackend.markSent()(line 112) andmarkFailureAttempt()(line 132) returning hardcoded1. TheOutboxBackendSPI Javadoc explicitly documents0for terminal-row no-ops. Code confirms both methods return1unconditionally. Merged A and B bodies; kept the defer note.A3 (brittle test assertion, unique-to-A) → KEPT. Line 104 of
OutboxBackendTest.javaconfirmed:.hasMessageContaining("synchronization is not active")pins a Spring-internal exception string. The load-bearing negative assertion is.doesNotContain("MUST share the business transaction"); the positive pin adds no contract value and is fragile across Spring upgrades.B2 (outboxPublishCompletion unconditionally wired, unique-to-B) → KEPT. Lines 209–212 confirm
outboxPublishCompletioncarries no@ConditionalOnMissingBeanor property condition, whileoutboxBackendat line 229 does carry@ConditionalOnMissingBean. A consumer who supplies a customOutboxBackend(e.g. Phase 0b Redis) will still triggerOutboxPublishCompletioncreation, which requiresOutboxRecordRepository(JPA). This defeats the extensibility seam's purpose at Phase 0b bootstrap. Finding structurally verified.Final tally: kept 4 findings (1 agreed, 3 unique-to-one verified). All severity=minor. No findings dropped.
Blast Radius
All changes are confined to a single Maven module (
outbox-publisher). The newOutboxBackendSPI is an exported interface but is only consumed within the same module byOutboxPublisherandOutboxPollerWorker. The@ConditionalOnMissingBeanwiring changes affect any downstream consumer that imports the auto-configuration, but PG behaviour is entirely preserved in Phase 0a.BLAST_SCORE: 4/10
CI status (head
eb800eee586e)Overall: ✗ failure
2 checks: 2 pending
Findings (3)
[MINOR] outboxBackend() factory never reads properties.getBackend() — im2be.outbox.backend=REDIS silently uses PG with no startup warning
outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:229
The
outboxBackend()factory (lines 227–234) accepts onlyOutboxRecordRepositoryandOutboxPublishCompletion; it does not acceptOutboxPropertiesand never callsproperties.getBackend(). An operator who setsim2be.outbox.backend=REDISwill receivePgOutboxBackendsilently — Spring's relaxed binding accepts the enum value without error, the context starts normally, and there is no log or exception to diagnose the mismatch.The
Backend.REDISJavadoc at lines 112–119 ofOutboxProperties.javadoes note "selecting it today has no effect," but that caveat is only visible to operators reading enum source — not from the property reference.Suggested fix: inject
OutboxPropertiesintooutboxBackend()and emit a startup warning whenbackend != Backend.PG:This makes misconfiguration immediately visible in startup logs with zero runtime-behaviour change.
[MINOR] markSent() and markFailureAttempt() always return 1 — violates the OutboxBackend SPI's documented 0-for-no-op contract
outbox-publisher/src/main/java/com/aim2be/platform/outbox/PgOutboxBackend.java:112
The
OutboxBackendSPI Javadoc explicitly documents: returns 0 when the row is already terminal (idempotent no-op). However,PgOutboxBackend.markSent()(line 112) andmarkFailureAttempt()(line 132) both return the hardcoded literal1unconditionally, becauseOutboxPublishCompletionreturnsvoid.This creates a behavioural asymmetry: a Phase 0b Redis implementation that correctly returns
0for racing terminal-row transitions will behave differently under any future orchestration code that branches on the return value (metrics, dead-letter diagnostics). TheFakeBackendinOutboxBackendTestalso hardcodesreturn 1, reinforcing the pattern in test code copied by Phase 0b implementers.PgOutboxBackendTestonly assertsrowcount == 1(lines 121 and 132); the already-terminal case is not exercised through the SPI.Recommended fix (pick one):
markSentandmarkFailureAttempttovoid— since nothing in Phase 0a branches on the value, eliminating the@returnspecification removes the contract entirely.OutboxPublishCompletion.markSent/markFailureAttemptto returnint(the rowcount from the JPQL@Modifyingquery) and propagate it throughPgOutboxBackend.At minimum, add
// TODO(Phase 0b)comments at lines 112 and 132 explicitly noting the hardcoded value.[MINOR] Assertion pins Spring-internal exception message text — brittle across Spring upgrades
outbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxBackendTest.java:104
Line 104 asserts:
This string is taken directly from Spring Framework's
TransactionSynchronizationManagerinternal exception path. Spring does not treat internal exception messages as a stable API; a future Spring upgrade rewording the message (e.g. to"Transaction synchronization is not active"or"No active transaction synchronization") would silently break this assertion with a test failure unrelated to outbox logic.The load-bearing contract check is the negative assertion at line 106:
.doesNotContain("MUST share the business transaction")— it proves the publisher's tx-guard did NOT fire. The positivehasMessageContainingon the Spring-internal string adds no contract value.Suggested fix:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 3 findings (3m) • 2026-05-28T10:23:35.245Z → 2026-05-28T10:25:05.243Z • posted-as: pr-reviewer-bot
R1 verdict CONDITIONAL_APPROVE, findings (kept=4, all MINOR): (1) OutboxAutoConfiguration.java:229 — silent backend misconfiguration. outboxBackend() never read properties.getBackend(); setting im2be.outbox.backend=REDIS silently bound PG with no signal. (2) PgOutboxBackend.java markSent/markFailureAttempt — return-value contract. Both returned a hardcoded 1, contradicting the SPI's documented 0-for-already-terminal-no-op semantics. (3) OutboxBackendTest.java:104 — brittle assertion pinning Spring's internal "synchronization is not active" message (not a stable API). (4) (arbitration B2) outboxPublishCompletion bean unconditionally wired — would force JPA wiring on a Phase 0b Redis-only consumer. Fix: - (1) inject OutboxProperties into outboxBackend() + emit a startup WARN when backend != PG (falls back to PgOutboxBackend). Misconfig now visible in logs; zero runtime-behaviour change. - (2) make OutboxPublishCompletion.markSent/markFailureAttempt return the repository @Modifying rowcount (was void; the value was already captured for the DEBUG no-op log, just discarded), and have PgOutboxBackend propagate it. The SPI's 1/0 contract is now honoured truthfully by PG — and uniformly with the Phase 0b Redis backend, which naturally returns 0/1 for racing terminal transitions. Chose the reviewer's "honest int" alternative over the "void" preferred option: it preserves the forward-looking contract instead of deleting it. Orchestration still discards the value → zero behaviour change. - (3) drop the positive message pin; keep the load-bearing NEGATIVE assertion (.doesNotContain("MUST share the business transaction")) that proves the tx-guard did NOT fire. - (4) B2 is a real Phase 0b concern; its proper fix is gating the whole PG bean graph (repository + completion + PG backend) on backend selection / JPA presence — which is exactly Phase 0b's module-split scope per the design. Added a TODO(Phase 0b) on the completion bean documenting this; deferred (a half-fix in Phase 0a would be incorrect). Ripple (rule 63): the void→int change broke an anonymous OutboxPublishCompletion subclass in OutboxPublisherTest (the OTel span-context spy) — updated its markSent override to return int. Added PgOutboxBackendTest cases exercising the already-terminal 0-rowcount path through the SPI (reviewer noted it was untested); added OutboxPublishCompletionTest rowcount assertions; annotated the FakeBackend's arbitrary return so Phase 0b implementers don't copy it as a rowcount model. Verification: - mvn -pl outbox-publisher -am clean verify → BUILD SUCCESS, 0 [WARNING] - 96 unit + 4 IT green; 27 OutboxPublisherTest characterization tests unchangedShow previous round
hib-pr-reviewer review — PR #7 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
af8bd3839531, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 1 verified minor finding kept (unique-to-A cross-test coupling); all 3 Round-1 blockers confirmed fixed by both reviewers; no blocking or major issues remain.
Summary
Arbitration — Round 2
Approach: Reviewer A surfaced 1 unique-to-A minor finding (cross-test-class coupling at
OutboxBackendTest.java:131); Reviewer B found nothing new. Per Rule 2 I usedReadto verify the cited file and line before deciding.Verification result: Confirmed.
OutboxBackendTest.java:131containsnew OutboxPublisherTest.SynchronousTestTxManager()verbatim.SynchronousTestTxManageris declared as a package-privatestatic final classnested insideOutboxPublisherTest(line 132 of that file, nopublicmodifier). Both classes share thecom.aim2be.platform.outboxtest package so it compiles today, but any rename, move, or visibility change toOutboxPublisherTestwill silently breakOutboxBackendTestat compile time.Outcome: 1 finding kept (verified unique-to-A minor); Reviewer B's empty set stands otherwise. All three Round 1 findings were resolved per both reviewers' independent confirmation — no prior regressions detected.
Blast Radius
The diff is scoped to a single Maven module (
outbox-publisher) and introduces a new storage SPI interface plus its default PG implementation. The changed surface is exported (theOutboxBackendinterface is public and@ConditionalOnMissingBean-guarded), so consumers who extend it are affected, but the orchestration contract is unchanged for existing PG users. No cross-module protocol changes or migration files are touched.BLAST_SCORE: 4/10
Risk Indicators
OutboxAutoConfiguration.outboxBackend,OutboxPublishCompletion.markSent,OutboxPublishCompletion.markFailureAttempt,PgOutboxBackend.markSent,PgOutboxBackend.markFailureAttemptCI status (head
af8bd3839531)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR] Cross-test-class coupling:
OutboxPublisherTest.SynchronousTestTxManagerreferenced directlyoutbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxBackendTest.java:131
Line 131 constructs
new OutboxPublisherTest.SynchronousTestTxManager()insideOutboxBackendTest.SynchronousTestTxManageris a package-privatestatic final classnested insideOutboxPublisherTest(declared at line 132 of that file with nopublicmodifier). Both test classes currently share thecom.aim2be.platform.outboxpackage so this compiles, but the coupling is fragile: renaming, moving, or tightening visibility onOutboxPublisherTestsilently breaksOutboxBackendTestat compile time with no obvious connection between the two changes. The clean fix is to extractSynchronousTestTxManagerinto a dedicated test-support type (e.g. aTestTransactionManagershelper or anOutboxTestSupportclass in the same test package) and have both test classes reference it there.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 2 • 1 finding (1m) • 2026-05-28T10:37:05.382Z → 2026-05-28T10:37:41.172Z • posted-as: pr-reviewer-bot
R2 verdict CONDITIONAL_APPROVE; all 3 R1 findings confirmed fixed by both reviewers. 1 new finding (kept=1, MINOR): (1) OutboxBackendTest.java:131 — cross-test-class coupling. The test referenced OutboxPublisherTest.SynchronousTestTxManager, a package-private nested helper in the other test class; a rename/move/visibility change to OutboxPublisherTest would silently break OutboxBackendTest at compile time. (Pre-existing in the Phase 0a test code; R1 had larger findings.) Fix: promote SynchronousTestTxManager to a top-level package-private test-support class (new SynchronousTestTxManager.java) in the same test package. OutboxPublisherTest's 15 in-file usages now bind to the top-level type (same package, no source change at the call sites); OutboxBackendTest drops the OutboxPublisherTest. qualifier. Neither test reaches into the other's internals anymore. Ripple (rule 63): removing the nested class orphaned 3 imports in OutboxPublisherTest (AbstractPlatformTransactionManager, TransactionDefinition, DefaultTransactionStatus) — removed them. No assertion changed in any of the 27 characterization tests. Verification: - mvn -pl outbox-publisher -am clean verify → BUILD SUCCESS, 0 [WARNING] - 96 unit + 4 IT green; 27 OutboxPublisherTest characterization tests unchangedShow previous round
hib-pr-reviewer review — PR #7 (affinity-intelligence-rework/im2be-platform-libs)
Round 3 — head
464e302f88fc, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 0 agreed findings (A found nothing); 1 unique-to-B minor/deferred finding verified as factually grounded and kept; no blocking issues.
Summary
Reconciliation — Round 3
Reviewer A found 0 issues. Reviewer B raised 1 unique-to-B minor/deferred finding about
requiresActiveTransaction()method naming.Verification performed:
OutboxBackend.javalines 130–144: confirmed the method exists at line 143, and the Javadoc scope is limited only to thepersistPendingguard, not the relay registration step.OutboxPublisher.javaforregisterSynchronization: confirmed it is called unconditionally at line 362, insidepersistAndRegisterRelay, regardless ofrequiresActiveTransaction()return value.OutboxBackendTest.javalines 85–104: confirmed the test comments explicitly document this gap (Phase 0b concern, relay registration still throws in tx-free context).Finding verdict: The B finding is factually grounded and verified. The SPI method name
requiresActiveTransaction()implies the entirepublish()flow can run outside a transaction when it returnsfalse, butregisterSynchronization(...)at line 362 ofOutboxPublisher.javais unconditional and will still throwIllegalStateException("Transaction synchronization is not active")for any backend returningfalse. The Javadoc narrows scope only in prose; the method name does not. The test class documents this at lines 88–96, but the SPI interface itself carries no such warning. The finding is correctly tagged as deferred/minor (no Redis backend in Phase 0a), so no blocking issue exists. Kept as-is.Blast Radius
The diff touches the outbox module's internal SPI boundary (
OutboxBackend,OutboxPublisher,OutboxPollerWorker,OutboxAutoConfiguration,OutboxProperties). Impact is confined to the singleoutbox-publisherMaven module. The new SPI is@ConditionalOnMissingBean, so consumers who supply their own backend are unaffected; those relying on Spring Boot autoconfiguration get the same PG behaviour as before with zero observable change in Phase 0a.BLAST_SCORE: 4/10
CI status (head
464e302f88fc)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR] requiresActiveTransaction() name is broader than its actual scope — Phase 0b implementors will be surprised
outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxBackend.java:143
The Javadoc correctly limits this method to controlling whether
persistPendingneeds an active Spring transaction. However, the method namerequiresActiveTransaction()reads as though the entirepublish()invocation can run outside a Spring transaction when the method returnsfalse. In reality,OutboxPublisher.persistAndRegisterRelay()(line 362 ofOutboxPublisher.java) callsTransactionSynchronizationManager.registerSynchronization(...)unconditionally, regardless of this flag. A Phase 0b Redis-backend implementor who returnsfalsewill findpublish()still throwsIllegalStateException("Transaction synchronization is not active")from Spring's relay-registration step — a different failure, at a different spot, with no indication from the SPI that this is expected.The
OutboxBackendTest.javacomment (lines 88–96) does document this limitation, but only in the test class; the SPI itself carries no such warning. To avoid this surprise in Phase 0b, consider renaming topersistRequiresActiveTransaction()(narrows the scope lexically) or adding a prominent@apiNotetorequiresActiveTransaction()stating: "this flag only gates thepersistPendingguard; the AFTER_COMMIT relay registration at the call site independently requires an active Spring synchronization context — a fully transaction-free backend also needs the relay registration step to be decoupled (Phase 0b concern)."Deferred: Phase 0b concern — no Redis backend exists in Phase 0a; renaming the SPI now would be a source-incompatible change that should land together with the Redis module. Label:
tech-debt.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-05-28T10:43:58.066Z → 2026-05-28T10:44:34.579Z • posted-as: pr-reviewer-bot
R3 verdict CONDITIONAL_APPROVE; 0 agreed findings. 1 unique-to-B finding, explicitly tagged Deferred/tech-debt (kept=1, MINOR): (1) OutboxBackend.java:143 — requiresActiveTransaction()'s name reads broader than its scope. It gates only the persistPending precondition; the AFTER_COMMIT relay registration (OutboxPublisher.persistAndRegisterRelay, registerSynchronization) is unconditional, so a backend returning false run outside a Spring tx still fails at the relay step. The reviewer offered either (a) a source-incompatible rename to persistRequiresActiveTransaction() or (b) an @apiNote — and deferred the rename to land with the Redis module. Fix: add the non-breaking @apiNote (option b) documenting that the flag gates ONLY the persist guard, that registerSynchronization is unconditional, and that both the relay-decoupling and the possible rename are Phase 0b concerns. This makes the SPI contract honest for the Phase 0b implementor now, without the source-incompatible rename (which lands with the Redis backend that needs the relay decoupled anyway). Verification: - mvn -pl outbox-publisher -am clean verify → BUILD SUCCESS, 0 [WARNING] - 96 unit + 4 IT green; 27 OutboxPublisherTest characterization tests unchangedhib-pr-reviewer review — PR #7 (affinity-intelligence-rework/im2be-platform-libs)
Round 4 — head
9ae94a7fbdaf, basemain, triggersynchronizeTL;DR: NO_NEW_FINDINGS — No new findings this round.
Summary
[quorum-converged] A=0 = B=0. # hib-pr-reviewer/quorum/A — round 4
Reviewing PR #7 in
affinity-intelligence-rework/im2be-platform-libs(head
9ae94a7, basemain).CI status (head
9ae94a7fbdaf)Overall: ✗ failure
2 checks: 2 pending
Findings
No new findings this round.
Quorum converged on empty findings (A + B both returned 0).
Verdict
NO_NEW_FINDINGS
hib-pr-reviewer • round 4 • 0 findings • 2026-05-28T10:46:45.422Z → 2026-05-28T10:50:00.691Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]