feat(outbox): PR-PLATFORM-1 — outbox-publisher full impl per ADR-0014 #4
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/pr-platform-1-outbox-publisher"
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?
Summary
Replaces the v1.0 scaffold's
UnsupportedOperationException("v1.0 scaffold")stubs inoutbox-publisher/with a production-ready transactional outbox publisher per ADR-0014 D-1 + D-9 (dual-mode relay, locked 2026-05-14).What is in
OutboxRecord— extended withlast_error(1000-char truncating setter),(status, created_at)+topicindexes,Status.isTerminal()helper. Compound PK(aggregateId, eventId)preserved from the v1.0 scaffold (rule 10 — match existing patterns; matches ADR-0014 D-5 UUIDv7 idempotency key).OutboxRecordRepository—Page<>PENDING-row fetch (poller batch), backlog-age JPQL gauge query,markSent/markFailureAttemptatomic JPQL updates.OutboxPublisher— hot AFTER_COMMIT relay wrapped in Resilience4jCircuitBreaker(im2be-outbox-relay, name pinned). Runtime-guards on active transaction (rule 18 + ADR-0014 D-7). OTel spanim2be.outbox.publish.hot.OutboxPollerWorker—@Scheduledcold path, ShedLock 6.0.2 distributed lock (outbox-poller), synchronoussend().get()per row. OTel spanim2be.outbox.poller.tick.OutboxPublishCompletion— REQUIRES_NEW transactional helper so post-commitmarkSent/markFailureAttemptwrites execute in their own transaction (the AFTER_COMMIT parent txn is already committed).UuidV7— stdlib-free UUIDv7 generator (Java 17 lacks v7). Required for time-orderedevent_idper ADR-0014 D-5.OutboxMetricsBinder— four Micrometer metrics per OQ-5:im2be_outbox_pending_total,im2be_outbox_backlog_age_seconds,im2be_outbox_publish_latency_seconds(per-topic timer),im2be_outbox_publish_failure_total(per-topic counter).OutboxProperties—@ConfigurationProperties(prefix="im2be.outbox")for poller cadence/batch + circuit-breaker tuning.OutboxAutoConfiguration— wiresCircuitBreakerRegistry+ ShedLockJdbcTemplateLockProvider+ fallbackOpenTelemetry.noop().Test plan
@SpringBootTest @Testcontainerswithpostgres:15-alpine+spring-kafka-test EmbeddedKafkaBroker: happy path, runtime-guard, poller sweep.mvn -B clean installandmvn -B clean verifyboth BUILD SUCCESS with zero[WARNING]lines (rule 62).Pinned dependency choices (rule 9 + rule 61)
io.github.resilience4j:resilience4j-circuitbreakernet.javacrumbs.shedlock:shedlock-springnet.javacrumbs.shedlock:shedlock-provider-jdbc-templateio.opentelemetry:opentelemetry-apiprovidedscope — consumer wires the SDK.org.testcontainers:postgresql+junit-jupiterCross-references
Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
c3a1062db000, basemain, triggeropenedTL;DR: NEEDS_WORK — 2 major + 4 minor + 1 info findings kept this round.
Summary
Arbiter reconciled 3 (A) + 5 (B) → 7 findings.
CI status (head
c3a1062db000)Overall: ✗ failure
2 checks: 2 pending
Findings (7)
[MAJOR] Circuit breaker is blind to async Kafka ack failures — only synchronous throws count against failure rate
outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:238
Lines 238-239:
sendAndDispatchCompletion(line 274) callskafkaTemplate.send(...).whenComplete(...)and returns immediately — the future is never awaited.executeRunnabletherefore observes a clean synchronous return for every case where the producer can locally enqueue the record. Async nacks — leader failover, broker quota, replication timeout — are never counted against the breaker failure rate and will never trip it OPEN. Only synchronous throws fromsend()itself (serializer error, oversized record) count, which are a tiny fraction of real-world broker failures.The Javadoc at lines 261-264 even acknowledges this: "async send failures are surfaced via the
whenCompletecallback" — but that callback is never wired back to the breaker.Fix option A — use
CircuitBreaker.decorateCompletionStage:Fix option B — call
circuitBreaker.onError/circuitBreaker.onSuccessmanually inside the existingwhenCompletecallback and removeexecuteRunnable.[MAJOR]
Status.FAILEDis never written — undeliverable rows retry on every tick forever, starving the batchoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:184
persistFailureAttempt(lines 184-203) incrementsretriesand storeslastErrorbut contains no retry-budget check.OutboxProperties.Pollerhas nomaxRetriesfield. Confirmed by grepping the entire module: no code path inOutboxPollerWorker,OutboxPublisher,OutboxPublishCompletion, orOutboxRecordRepositoryever writesStatus.FAILED.OutboxRecord.Status.FAILEDis defined at line 71 with Javadoc "exceeded retry budget; awaits operator inspection" but this contract is never fulfilled.Consequences:
message.max.bytes) occupies one slot in every poller batch tick indefinitely.batch-size(default 500), newer PENDING rows are deferred — theORDER BY created_at ASCFIFO sweep means old poisoned rows always come first.im2be_outbox_pending_totalgauge never drains; the ADR-0014 §D-8 alert fires continuously.Fix: add
maxRetries(e.g. default 10) toOutboxProperties.Poller; inpersistFailureAttemptcheckrecord.getRetries() + 1 >= maxRetries— if true, call a newrepository.markFailed(aggregateId, eventId)JPQL update settingstatus = FAILED. The poller queries onlystatus = PENDING, so FAILED rows are naturally excluded from future ticks.[MINOR] Unbounded
.get()on cold-path Kafka send can stall the poller thread indefinitelyoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:168
get()without a timeout blocks until theCompletableFutureresolves. If the Kafka broker is wedged such that the future never completes (distinct from returning anExecutionException), the poller thread hangs for the full ShedLock hold window (lock-at-most, default PT1M). During that window, cold-path recovery stalls for all services sharing the same replica.Fix: replace with a timed
getsourced from the producer's delivery timeout:Add
deliveryTimeoutMs(default 30 000) toOutboxProperties.Poller. The resultingTimeoutExceptionis caught by the existingcatch (Exception sendError)block.[MINOR]
ExecutionExceptionstored inlast_errorwithout unwrapping the root causeoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:178
kafkaTemplate.send().get()throwsExecutionExceptionwrapping the actual Kafka exception. When caught at line 178 (catch (Exception sendError)),persistFailureAttemptbuilds the error string at line 186 ascause.getClass().getSimpleName() + ": " + cause.getMessage(). The storedlast_errorwill therefore read"ExecutionException: org.apache.kafka.common.errors.TimeoutException: ..."instead of"TimeoutException: ...". Operators querying the table for triage see the wrapper class, not the root cause.Fix: In
persistFailureAttempt, unwrapExecutionExceptionbefore building the message:[MINOR] Narrow
DataAccessExceptioncatch letsCannotCreateTransactionExceptionescape frompersistFailureAttemptoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:306
Line 306:
completion.markFailureAttempt()runs in aREQUIRES_NEWtransaction. If the connection pool is exhausted or the datasource is unavailable, Spring throwsCannotCreateTransactionException(aTransactionException), which is NOT a subclass ofDataAccessException. That exception propagates out of theafterCommit()callback into Spring's transaction synchronisation infrastructure, where behaviour is undefined across versions (Spring may log-and-swallow or re-throw).Fix:
or at minimum:
[MINOR] OTel span outcome set to
successbefore async ack arrives; span is closed beforewhenCompletefiresoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:240
Line 240 sets
span.setAttribute(ATTR_OUTCOME, "success")immediately afterexecuteRunnablereturns — which, per finding B-1, happens before the Kafka ack. Thefinallyblock at lines 254-256 closes the span before thewhenCompletecallback runs on the Kafka I/O thread. Kafka async failures therefore produce spans withoutcome=success, breaking theim2be.outbox.publish.hottrace for the dominant failure mode.Fix: Move the span start/end scope inside the
whenCompletecallback so the outcome attribute and span end reflect the actual async result. Alternatively, record outcome as a separate OTel event from withinwhenCompleteand keep the parent span open until then (requires passing the span reference into the callback and callingspan.end()from there).[INFO] Unbounded
Listoverload offindByStatusOrderByCreatedAtAscis a public footgunoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxRecordRepository.java:134
The Javadoc warns "callers that need bounded fetch MUST use the
Pagevariant" but the unbounded overload remainspublicon the interface. A caller that accidentally imports this signature instead of thePageablevariant will issue aSELECT * FROM outbox_records WHERE status = 'PENDING' ORDER BY created_atagainst a potentially large table and load the full result set into heap. This risk is amplified by the backlog scenario in finding B-2 (thousands of permanently-stuck PENDING rows).Fix: Either remove the overload entirely (breaking-change flag before first GA) or annotate it
@Deprecatedwith a Javadoc@deprecateddirecting callers to the bounded variant — making the footgun visible at compile time.Verdict
NEEDS_WORK
hib-pr-reviewer • round 1 • 7 findings (2M/4m/1i) • 2026-05-26T22:45:56.190Z → 2026-05-26T22:52:26.394Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]
Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
70271161aff4, basemain, triggersynchronizeTL;DR: NEEDS_WORK — kept 4 findings (1 major, 2 minor, 1 info); all unique-to-one-reviewer but individually verified against HEAD; B-1 status-guard omission is a live correctness race that can silently promote FAILED rows to SENT.
Summary
Arbitration — Round 2
No prior Memora history found (Memora persistence encountered tag-allowlist restrictions; run summary write was attempted). This is treated as the first recorded arbitration for this PR, though the PR itself is at round 2 with all 7 round-1 findings already resolved by the author.
Approach: All 4 findings (2 unique-to-A, 2 unique-to-B) required individual verification via
Read. None overlap between A and B.A-unique findings
OutboxPollerWorker): CONFIRMED.tryColdPublishcatchesInterruptedException, re-sets the flag withThread.currentThread().interrupt(), then returnsfalseto the for-loop inpollAndPublishPending(line 141). The loop has no guard; every subsequentkafkaTemplate.send(...).get(sendTimeoutMs, MILLISECONDS)call on remaining batch records will see the sticky interrupt flag and throwInterruptedExceptionimmediately, triggeringpersistFailureOutcomeon work never attempted. Kept.OutboxPublisher): CONFIRMED.scopeopened at line 274,whenCompleteregistered at lines 276–284,scope.close()fires in the synchronousfinallyblock at line 291 — before the Kafka I/O thread ever invokes the callback.handleAsyncAcktherefore runs with no active OTel context on the I/O thread; child spans frommarkSent/markFailureAttemptinside it are trace roots. Kept.B-unique findings
OutboxRecordRepository): CONFIRMED.markSentWHERE clause (lines 87–89):WHERE o.aggregateId = :aggregateId AND o.eventId = :eventId— no status predicate.markFailureAttempt(lines 112–114): same. The Javadoc at line 83 claims the method returns 0 for already-terminal rows, but the query does not enforce that. Two distinct races are live: (a) delayed hot-relay success ack can promote a FAILED row back to SENT; (b) poller winsmarkSent, then hot-relay error callback bumpsretries/lastErroron the now-SENT row.@Version-based optimistic locking does not protect JPQL bulk updates. Kept.OutboxProperties): CONFIRMED. No@Validatedannotation on the class. FieldsbatchSize,sendTimeoutMs,failureRateThresholdcarry no@Min/@Maxconstraints; invalid values silently bind and cause obscure runtime failures. B explicitly deferred this to a follow-on PR. Kept (minor/deferred).Summary: 4 findings kept (1 major, 2 minor, 1 info). B-1 is a correctness gap that can silently corrupt terminal row states in production; it is the blocking concern.
Blast Radius
The diff touches 19 files across the entire outbox-publisher module — publisher, poller, repository, completion helper, metrics binder, auto-configuration, and properties. This is the core library consumed by all 7 aim2be JPA services. The status-guard omission (B-1) affects every row that traverses the hot+cold relay concurrently, which is the normal operating path under any broker hiccup.
BLAST_SCORE: 6/10
Risk Indicators
markSent,markFailureAttempt,markFailed,tryColdPublish,handleAsyncAck,persistFailureOutcomeCI status (head
70271161aff4)Overall: ✗ failure
2 checks: 2 pending
Related PRs
Findings (4)
[MAJOR] markSent and markFailureAttempt lack a status guard — delayed hot-relay callbacks can corrupt terminal rows
outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxRecordRepository.java:89
Both JPQL bulk-update queries match only on
(aggregateId, eventId)with no status predicate:The Javadoc on
markSent(line 83) claims the method returns 0 rows updated when the row is already terminal, but the query does not enforce this.Race A — FAILED → SENT resurrection: With
maxRetries=10andpoller.interval-ms=5000the poller can exhaust the retry budget and writeFAILEDin ~50 s. The hot relay'swhenCompletefires within the Kafka producer'srequest.timeout.ms(default 30 s, configurable higher). If the ack arrives after budget exhaustion: poller writesFAILED→ operator is alerted → hot-relay ack arrives →completion.markSentexecutesmarkSent→ WHERE clause matches the FAILED row → row silently promoted toSENTwith no log entry.Race B — SENT row retries corruption: Poller wins
markSentfirst; row isSENT. Hot-relay callback fires withsendError != null→persistFailureAttempt→markFailureAttemptincrementsretriesand writeslastErroron the already-SENTrow.Note:
@Version-based optimistic locking does NOT protect JPQL bulk updates — it only applies to entity-levelsave/mergeoperations.Fix: Add a status predicate to both queries:
With these guards a delayed second write affects 0 rows, matching the idempotency the Javadoc already promises.
[MINOR] InterruptedException re-sets flag but loop continues — spurious retry increments on shutdown
outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:192
After
Thread.currentThread().interrupt()the sticky interrupt flag is set on the thread.pollAndPublishPending(line 141) continues iterating the remaining records inbatch. Every subsequentkafkaTemplate.send(...).get(sendTimeoutMs, MILLISECONDS)call will immediately throwInterruptedException— triggeringpersistFailureOutcomeon each remaining row. On a graceful JVM shutdown with a large batch, many rows receive a spuriousretries++andlastErrorupdate for work that was never attempted.Fix — add an interrupt guard at the loop entry in
pollAndPublishPending:[MINOR] OutboxProperties has no JSR-303 validation — out-of-range values cause confusing runtime failures
outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxProperties.java:30
The
@ConfigurationPropertiesclass carries no@Validatedannotation and no field-level@Min/@Max/@Positiveconstraints. The following bad values silently pass binding and then blow up at call-site runtime:poller.batch-size=0PageRequest.of(0, 0)IllegalArgumentException: Page size must not be less than onepoller.send-timeout-ms=0.get(0, MILLISECONDS)TimeoutExceptionon every cold sendcircuit-breaker.failure-rate-threshold=0CircuitBreakerConfig.custom().failureRateThreshold(0)IllegalArgumentExceptionfrom Resilience4j at startupcircuit-breaker.failure-rate-threshold=101Note: B deferred this finding to a follow-on hardening PR since defaults are all valid and there is no correctness regression introduced in this PR. Recommended fix is to add
@Validatedto the class and@Min/@Maxconstraints to affected fields.[INFO] OTel context not propagated into the Kafka I/O callback — DB child spans appear orphaned in traces
outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:277
scope.close()removes the span from the calling thread's OTel context in the synchronousfinallyblock. The Kafka I/O thread that invokeswhenCompletehas no active OTel context. WhenhandleAsyncAckcallscompletion.markSent(...)orcompletion.markFailureAttempt(...), any child spans those methods create appear as trace roots, disconnected from theim2be.outbox.publish.hotparent.This is not a correctness regression — the round-1 span-lifecycle fix (ending the span inside
handleAsyncAck) was necessary and correct; this is the follow-on observability gap that fix exposed.Fix — capture the context before the scope closes and wrap the callback:
Verdict
NEEDS_WORK
hib-pr-reviewer • round 2 • 4 findings (1M/2m/1i) • 2026-05-27T00:05:22.793Z → 2026-05-27T00:08:11.217Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 3 — head
e039f4ba8550, basemain, triggersynchronizeTL;DR: BLOCKED — kept 3 findings (1 blocking @EntityScan entity-scan clobber, 1 major MeterRegistry no-guard, 1 minor isTerminal API footgun); all unique-to-one reviewer but each verified by direct Read of the cited file:line.
Summary
Arbitration — Round 3
Memora persistence was attempted but the server rejected all tag configurations (
memora/issuesnot in allowlist;memory_absorbrequired an additional permission grant); the Memora step is a best-effort annotation and does not affect the verdict.Verification performed
@EntityScanclobbers consumer entity scanningOutboxAutoConfiguration.java@EntityScan(basePackageClasses = OutboxRecord.class)present at line 65; noImportBeanDefinitionRegistrarworkaroundoutboxMetricsBinderunconditionalMeterRegistrydepOutboxAutoConfiguration.javaMeterRegistryis a required constructor arg; no@ConditionalOnBeanguard;micrometer-coreisprovidedin pom.xmlisTerminal()returnsfalseforFAILEDOutboxRecord.javareturn this == SENT || this == ARCHIVED; Javadoc at 75–79 explains intent but method name creates downstream footgunAll three findings are unique-to-one reviewer but all three are grounded in the actual source at the cited lines. Per Rule 2 they are kept.
No prior-round findings re-raised (all 11 R1+R2 findings confirmed resolved by both reviewers; arbitration defers to that shared assessment).
Blast Radius
This is a shared library consumed by all 7 aim2be JPA services. The
@EntityScandefect strikes at JPA bootstrap and will silently drop every consumer's own domain entities from Hibernate scanning the moment the library is adopted — a fleet-wide breakage at startup. TheMeterRegistrydefect gates context startup for any consumer not running Actuator. Both issues manifest before any business code runs.BLAST_SCORE: 8/10
Risk Indicators
outboxMetricsBinder,outboxLockProvider,OutboxPollerWorker.pollAndPublishPendingCI status (head
e039f4ba8550)Overall: ✗ failure
2 checks: 2 pending
Findings (3)
[BLOCKING]
@EntityScan(basePackageClasses = OutboxRecord.class)silently drops consuming-service entities from Hibernate scanningoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:65
Line 65:
@EntityScan(basePackageClasses = OutboxRecord.class)is present on the auto-configuration class.Spring's
JpaBaseConfiguration.getPackagesToScan()applies this logic:This annotation registers
["com.aim2be.platform.outbox"]intoEntityScanPackages. Because that list is now non-empty, theAutoConfigurationPackagesfallback — which carries the consuming service's root package — is never reached. Any of the 7 consuming services that do not declare their own@EntityScanwill have all their domain entities invisible to Hibernate at boot, producing silentEntityNotFoundException/MappingExceptionfailures in production.The integration test does not catch this because
ITConfig(@SpringBootApplication) lives in the same package asOutboxRecord(com.aim2be.platform.outbox).Fix — remove the
@EntityScanannotation and register the package additively via anImportBeanDefinitionRegistrar:This mirrors how Spring Data itself adds packages without clobbering the consumer's scanning.
[MAJOR]
outboxMetricsBinderhas an unconditional hard dependency onMeterRegistry; consuming services without Actuator cannot startoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:122
Lines 121–127:
micrometer-coreisprovidedscope inpom.xml(the consumer is expected to bringspring-boot-starter-actuator), but there is no@ConditionalOnBean(MeterRegistry.class)guard. If a consuming service enables the outbox (im2be.outbox.enabled=true) without Actuator on the classpath, Spring throwsNoSuchBeanDefinitionException: No qualifying bean of type 'MeterRegistry'at context refresh — before any business code runs.OutboxPublisherandOutboxPollerWorkerboth declareOutboxMetricsBinderas a required constructor argument, so the failure cascades.Fix option 1 — conditional + no-op fallback:
Fix option 2 — promote
spring-boot-starter-actuatortocompilescope inpom.xmland document it as a hard requirement, so the build fails fast for consumers that omit it.[MINOR]
isTerminal()returnsfalseforFAILED— misleading public API contract for downstream consumersoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxRecord.java:81
Lines 81–83:
The Javadoc at lines 75–79 defines the return contract as "the poller MUST NOT re-attempt the row" and notes that
FAILEDrows "await operator inspection". The intent is correct: the poller queries onlyWHERE status = PENDING, so FAILED rows are never auto-retried regardless ofisTerminal().However, the method name
isTerminal()conventionally means "nothing more will happen to this record automatically". A consumer in any of the 7 downstream services writingif (!record.getStatus().isTerminal()) { processRow(record); }will incorrectly include FAILED rows in an automated working set — rows that should be left for operator inspection or manual reset.Preferred fix — widen the condition and split the Javadoc intent:
and document
FAILEDseparately as "terminal but operator-resettable (→ PENDING)". Alternatively rename toisAutoRetriable()returningthis == PENDINGfor unambiguous poller semantics. At minimum, add a bold warning to the FAILED enum constant's Javadoc.Verdict
BLOCKED
hib-pr-reviewer • round 3 • 3 findings (1b/1M/1m) • 2026-05-27T00:26:11.452Z → 2026-05-27T00:28:30.870Z • posted-as: pr-reviewer-bot
R3 verdict findings (kept=3): (1) [BLOCKING] OutboxAutoConfiguration.java:65 — @EntityScan(basePackageClasses = OutboxRecord.class) OVERRIDES consumer's @SpringBootApplication entity scan. Spring's JpaBaseConfiguration.getPackagesToScan() consults EntityScanPackages first and falls back to AutoConfigurationPackages ONLY when EntityScanPackages is empty. Adding @EntityScan in a SHARED LIBRARY therefore silently drops every consumer-domain entity from Hibernate scanning at startup — fleet-fatal silent breakage. Fix: Remove @EntityScan; add new OutboxAutoConfigurationPackagesRegistrar implements ImportBeanDefinitionRegistrar that calls AutoConfigurationPackages.register(beanFactory, "com.aim2be.platform.outbox") — the supported library-side hook that APPENDS to the existing list rather than replacing it. @Import-ed from OutboxAutoConfiguration. (2) [MAJOR] OutboxAutoConfiguration.java:122 — outboxMetricsBinder bean takes MeterRegistry directly with no @ConditionalOnBean guard. micrometer-core is `provided` scope in pom.xml, so consumers without Spring Boot Actuator (or Micrometer on the classpath) fail context startup. The publisher + poller both depend on this bean unconditionally so making it conditional would cascade-break them. Fix: Switch outboxMetricsBinder ctor signature to take ObjectProvider<MeterRegistry> + pass meterRegistry.getIfAvailable() (nullable) to OutboxMetricsBinder. OutboxMetricsBinder gained a no-op mode: when constructed with a null registry, gauge registration is skipped and recordPublishLatency / recordFailure return immediately. Publisher + poller wiring unchanged. Coverage: new test nullRegistryYieldsNoOpBinder. (3) [MINOR] OutboxRecord.java:81 — Status.isTerminal() returned false for FAILED, contradicting: - OutboxPollerWorker:238 comment ("FAILED terminally") - The poller's actual behavior (fetches PENDING-only via findAllByStatusOrderByCreatedAtAscEventIdAsc; FAILED rows are never re-attempted automatically) - Naming intuition for "isTerminal" Fix: Include FAILED in isTerminal(). Updated Javadoc to clarify FAILED is terminal from the poller's perspective; operator inspection MAY manually reset FAILED → PENDING to re-enter the non-terminal lane. Test flipped: OutboxRecordTest now asserts FAILED.isTerminal() == true. Verification: - mvn -B -pl outbox-publisher test → 64 tests passed (3 new: registrar additivity + bare-consumer-contrast + null-registry no-op) - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] - New files: - outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfigurationPackagesRegistrar.java - outbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxAutoConfigurationPackagesRegistrarTest.java - outbox-publisher/src/test/java/com/aim2be/platform/outbox/fakeconsumer/FakeConsumerApplication.java Ripple-effect inspection (rule 63): - @EntityScan removal: OutboxAutoConfiguration's class Javadoc updated; @Import(...Registrar.class) replaces @EntityScan; the registrar runs as part of the autoconfig's BeanDefinitionRegistryPostProcessor pass, so JpaBaseConfiguration sees the appended package via AutoConfigurationPackages fallback. No other call sites to validate. - MeterRegistry conditional: outboxMetricsBinder's only callers (outboxPublisher + outboxPollerWorker) take the binder by type; they continue to work whether the binder is wired against a real registry or running in no-op mode. - isTerminal change: searched all callsites — single internal callsite is the OutboxRecordTest; updated. Memora #3531 captures the @EntityScan-as-shared-lib-anti-pattern lesson for future shared-Spring-Boot-library work.Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 4 — head
4e34df6dbeee, basemain, triggersynchronizeTL;DR: NEEDS_WORK — kept 2 unique-to-B findings (1 major circuit-breaker config bypass, 1 minor public API leakage); all R1-R3 findings confirmed closed; Reviewer A's @EnableJpaRepositories note not escalated (A explicitly dropped it from their own findings).
Summary
Reconciliation: Round 4
Reviewer A returned 0 findings (JSON
[]); A's prose noted a potential@EnableJpaRepositoriesshadow issue but explicitly chose not to raise it as a structured finding. Reviewer B returned 2 unique-to-A findings. Both were verified viaReadagainst HEAD.B1 (Major) — KEPT ✅
Verified in
OutboxAutoConfiguration.java(lines 81-93, 154-169) andOutboxPublisher.java(lines 127-141):outboxCircuitBreakerRegistryis@ConditionalOnMissingBean(line 82), so it is skipped when a consumer provides anyCircuitBreakerRegistrybean.outboxPublisherat line 158 injects whicheverCircuitBreakerRegistryis in context and passes it directly to theOutboxPublisher(registry,…)constructor overload.circuitBreakerRegistry.circuitBreaker(CIRCUIT_BREAKER_NAME)at line 137 — a Resilience4j lazy-create call that uses the registry's default config when noim2be-outbox-relaybreaker is pre-registered, silently ignoring everyim2be.outbox.circuit-breaker.*property the operator configured. The finding is accurate and the cited line matches reality.B2 (Minor) — KEPT ✅
Verified in
OutboxRecordRepository.java(lines 92-103) andOutboxPublishCompletion.java(lines 64-65):markSentis apublicinterface method accepting@Param("newStatus") OutboxRecord.Status newStatus.o.status = :newStatus, o.sentAt = :sentAt— a caller passingStatus.FAILEDwould persist a non-nullsentAton a never-delivered row.OutboxPublishCompletioncorrectly hard-codesStatus.SENTat its call site, but the public repository API exposes the unsafe overload to all downstream consumers.A's @EnableJpaRepositories note — NOT raised
A's prose noted this but explicitly filed no finding (JSON
[]). The annotation is scoped tobasePackageClasses = OutboxRecordRepository.class(line 70), limiting scan to the library's own package; theOutboxAutoConfigurationPackagesRegistrarhandles entity scanning additively. Dropping this per A's own verdict.Memora persistence attempted (3 calls); MCP permissions / tag allowlist prevented writes — run summary could not be stored.
Blast Radius
The major finding is in the library's auto-configuration wiring layer (
OutboxAutoConfiguration), which is the entry point for all 7 consuming services that opt in viaim2be.outbox.enabled=true. A consumer that provides their own Resilience4j registry — a common pattern in multi-library services — will silently receive a circuit breaker with default thresholds regardless of environment-tuned properties. The minor finding (OutboxRecordRepository) affects a public interface method but practical blast radius is limited sinceOutboxPublishCompletionis the sole intended caller within the library.BLAST_SCORE: 5/10
Risk Indicators
outboxCircuitBreakerRegistry,outboxPublisher,OutboxPublisher.<init>(OutboxRecordRepository,KafkaTemplate,CircuitBreakerRegistry,OutboxPublishCompletion,OpenTelemetry,OutboxMetricsBinder)CI status (head
4e34df6dbeee)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MAJOR]
im2be.outbox.circuit-breaker.*properties silently ignored when consumer provides their ownCircuitBreakerRegistryoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:163
The
outboxCircuitBreakerRegistrybean is@ConditionalOnMissingBean(line 82), so it is skipped when a consuming service already declares aCircuitBreakerRegistrybean. TheoutboxPublisherfactory (lines 154-169) injects whateverCircuitBreakerRegistryis present and passes it to theOutboxPublisher(registry,…)constructor overload (line 163). That overload callscircuitBreakerRegistry.circuitBreaker(CIRCUIT_BREAKER_NAME)atOutboxPublisher.java:137, which lazily creates theim2be-outbox-relaybreaker using the consumer registry's default configuration — silently ignoring everyim2be.outbox.circuit-breaker.failure-rate-threshold/wait-duration-in-open-state-msproperty the operator set.Fix: in
outboxPublisher(), always configure the outbox breaker explicitly:Add
OutboxProperties propertiesas a parameter tooutboxPublisher(). This is safe even when the library's own registry is in use — Resilience4j ignores the supplied config when the named breaker already exists, so theoutboxCircuitBreakerRegistrypre-registration remains a useful warm-up step but is no longer the sole config source.[MINOR]
markSentexposesnewStatusas a public parameter — allows callers to setsentAton a non-SENT transitionoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxRecordRepository.java:98
The method signature at lines 98-103 accepts
@Param("newStatus") OutboxRecord.Status newStatus. The JPQL at line 94 writeso.status = :newStatus, o.sentAt = :sentAtunconditionally; a caller passingStatus.ARCHIVEDorStatus.FAILEDwould persist a non-nullsentAton a row that was never successfully delivered.OutboxPublishCompletionhard-codesStatus.SENTat line 65, butOutboxRecordRepositoryis apublicinterface — any downstream consumer of this library can callmarkSent(id1, id2, Status.FAILED, Instant.now())without a compile-time error.Fix: remove the
newStatusparameter; hardcodeSENTin the JPQL:Update
OutboxPublishCompletion.markSent()to drop the extra argument.Verdict
NEEDS_WORK
hib-pr-reviewer • round 4 • 2 findings (1M/1m) • 2026-05-27T00:48:57.057Z → 2026-05-27T00:51:30.789Z • posted-as: pr-reviewer-bot
R4 verdict findings (kept=2): (1) [MAJOR] OutboxAutoConfiguration.java:81-93 + OutboxPublisher.java:127-141 — outboxCircuitBreakerRegistry is @ConditionalOnMissingBean; when a consumer provides their own CircuitBreakerRegistry, the breaker named "im2be-outbox-relay" gets lazy-created via the OutboxPublisher registry-overload constructor (registry.circuitBreaker(NAME)) with the consumer-registry's DEFAULT config, silently ignoring every im2be.outbox.circuit-breaker.* property. Fix: introduce a dedicated `outboxCircuitBreaker` bean that builds an EXPLICIT CircuitBreakerConfig from OutboxProperties and calls registry.circuitBreaker(NAME, config) — this creates-or-returns the named breaker with our explicit config regardless of registry source. The outboxPublisher bean now takes CircuitBreaker directly (not the registry). Library no longer imposes its outbox-specific thresholds on the registry's default config (the conditional registry bean reverts to ofDefaults()) so consumers' OTHER breakers in the same registry aren't accidentally configured against our policy. The registry-overload constructor on OutboxPublisher is REMOVED — the public API no longer carries the footgun shape. (2) [MINOR] OutboxRecordRepository.java:92-103 — `int markSent(aggregateId, eventId, @Param("newStatus") Status newStatus, sentAt)` accepts the target status as a method parameter; a caller passing Status.FAILED would persist a non-null sentAt on a never-delivered row. OutboxPublishCompletion hard-coded Status.SENT internally, but the public repository API exposed the unsafe overload. Fix: drop the newStatus parameter; hardcode OutboxRecord.Status.SENT inside the JPQL `SET o.status = ...SENT`. Callers no longer have any path to write sentAt with a non-SENT status. Distinct lifecycle transitions belong on distinct repository methods. Updated OutboxPublishCompletion + OutboxPublishCompletionTest stubs. Verification: - mvn -B -pl outbox-publisher test → 64 tests passed - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - CircuitBreaker bean rewiring: registry bean is now ofDefaults() to avoid contaminating consumer's other breakers with our 50% failure-rate default; outbox-specific config lives on the named breaker only. The OutboxPublisher direct-CircuitBreaker ctor was already present (the registry-overload was the offender), so the publisher's runtime behavior is unchanged when fed the same breaker — only the construction path differs. - markSent signature change: searched all callsites — OutboxPublishCompletion is the only caller in src/main; tests adjusted to match the new 3-arg overload. No external consumer was depending on the unsafe newStatus parameter at HEAD. - Removed OutboxPublisher's registry-overload constructor — its only autoconfig user is gone, and the tests now use the direct CircuitBreaker constructor matching the autoconfig path. Memora #3531 + #3524 capture the lessons catalogue.Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 5 — head
746ba9a66bc6, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 agreed finding (SpEL ShedLock bypass + README gap) and 1 verified unique-to-B finding (stale retries snapshot concurrency bug); no findings dropped; all 16 prior-round issues confirmed resolved.
Summary
Arbitration — Round 5
Memora note: Memory persistence failed (tool permission denied); run history not stored. Reconciliation proceeded on in-session evidence.
Verification performed
OutboxPollerWorker.javalines 115–165: confirmed@SchedulerLockat lines 121-123 reads bothlock-at-mostandlock-at-leastvia raw SpEL (${im2be.outbox.poller.lock-at-most:PT1M}/${im2be.outbox.poller.lock-at-least:PT0S}).OutboxProperties.java: grep forlockAtMost,lockAtLeast,lock-at-most,lock-at-leastreturned no matches — no typed fields, no JSR-303 constraints.README.mdline 157:lock-at-mostpresent in config table;lock-at-leastabsent.OutboxPollerWorker.javalines 250-255:final int currentRetries = record.getRetries()at line 253 confirmed as in-memory batch-load snapshot used forbudgetExhaustedevaluation.OutboxRecordRepository.javamarkFailureAttemptJPQL:SET o.retries = o.retries + 1— DB-side arithmetic, guarded only onstatus = PENDING, not on retries count. Confirms that a concurrent hot-relay call between batch fetch andpersistFailureOutcomecan push DB retries tomaxRetries + 1before the poller evaluates the stale budget.OutboxPublishCompletion.java: no additional guards on retries inmarkFailureAttemptpath.Reconciliation decisions
@SchedulerLockSpEL bypass / README gaplock-at-leastspecifically; B's body more technically accurate (SpEL-from-bean not viable for annotation constants → doc fix is primary mitigation)record.getRetries()at line 253markFailureAttemptJPQL confirms the one-over-budget raceNo prior-run findings re-raised. No findings dropped.
Blast Radius
Changes are confined to a single library module (
outbox-publisher) but the module is infrastructure shared by all 7 aim2be JPA services. Both findings are inOutboxPollerWorker— the cold-path sweep component — and do not touch the hot-relay or entity write paths. Blast radius is moderate: a misconfigured ShedLock duration or a stale-retries race would affect the poller tick on any consumer service, but the impact is bounded to one extra retry per window rather than data loss or unavailability.BLAST_SCORE: 4/10
Risk Indicators
pollAndPublishPending,persistFailureOutcome,markFailureAttempt,markFailedCI status (head
746ba9a66bc6)Overall: ✗ failure
2 checks: 2 pending
Related PRs
Findings (2)
[MINOR]
lock-at-mostandlock-at-leastbypassOutboxProperties/@Validated;lock-at-leastabsent from READMEoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:123
Neither
lock-at-mostnorlock-at-leasthas a typed field inOutboxProperties.Poller— a grep across the entireOutboxProperties.javafile returns no matches for either name. This means:@Validated/ JSR-303 protection. An invalid ISO-8601 string (e.g.lock-at-most=5000, missing thePTprefix) survives startup and only produces a crypticDateTimeParseExceptionfrom ShedLock on the first tick, not aBindValidationExceptionat context initialisation.lock-at-leastis entirely absent from the README configuration reference table (line 157 of README.md listslock-at-mostbut notlock-at-least), giving operators no documented discovery path.Important constraint:
@SchedulerLockannotation members are compile-time constants — they cannot source values via#{@outboxProperties.poller.lockAtMostFor}SpEL bean-reference syntax; only${property.key}placeholder resolution is available. Adding typed fields toOutboxProperties.Polleris therefore aspirational (you cannot wire the annotation member to the bean field at runtime). The actionable fix is documentation:lock-at-leastto the README configuration reference table alongsidelock-at-most.@SchedulerLockannotation noting that both values must be valid ISO-8601 Duration strings (e.g.PT30S, not30000).@PostConstructorSmartInitializingSingletonvalidation inOutboxPollerWorkerthat parses and validates the resolved placeholder strings at startup so misconfiguration surfaces before the first tick.[MINOR] Stale in-memory
retriessnapshot allows one over-budget retry under concurrent hot + cold relayoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:253
record.getRetries()reflects the value at the momentfindByStatusOrderByCreatedAtAscran. A concurrent hot-relaymarkFailureAttemptcall (SET o.retries = o.retries + 1— JPQL guarded only onstatus = PENDING, not on retries count) that fires between the batch fetch and this budget-exhaustion check increments the DB counter without the poller seeing it.Concrete race:
maxRetries = N).record.getRetries()→ N−1.markFailureAttemptfires: DB retries becomes N.(N-1) >= N→ false → callsmarkFailureAttemptagain → DB retries becomes N+1.maxRetriescontract.markFailed.Impact is bounded (self-corrects on next tick, row does not become permanently stuck), but the
maxRetriesproperty contract is silently violated by one attempt per stale-read window, andlast_errorreflects an extra failure attempt.Suggested fix: Move the budget-exhaustion decision to the JPQL layer so the DB evaluates the current
retriesvalue atomically:The return value of each (0 = no-op) tells the poller which transition was applied, eliminating the stale-snapshot race entirely.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 5 • 2 findings (2m) • 2026-05-27T01:02:25.798Z → 2026-05-27T01:05:13.485Z • posted-as: pr-reviewer-bot
R5 verdict findings (kept=2): (1) [AGREED A+B] OutboxPollerWorker:121-123 + README — @SchedulerLock reads `${im2be.outbox.poller.lock-at-most:PT1M}` and `${im2be.outbox.poller.lock-at-least:PT0S}` via raw property-placeholder SpEL (annotation values can NOT be bound from a @ConfigurationProperties bean at the annotation level — Spring resolves annotation strings via the Environment's PropertyResolver). OutboxProperties.Poller had NO typed fields for either knob and README.md documented only `lock-at-most`. Fix: added typed `String lockAtMost` (default "PT1M") + `String lockAtLeast` (default "PT0S") to OutboxProperties.Poller with @Pattern JSR-303 validation enforcing ISO-8601 duration format (PT30S, PT1M, PT5M30S, etc.). The typed fields are the documentation + binding-time validation surface — operators see them in the bound OutboxProperties bean and an `im2be.outbox.poller.lock-at-most=not-a-duration` startup property fails binding fast instead of silently propagating to @SchedulerLock as raw text. Added 3 new property-validation tests. README config table updated to document `lock-at-least` (plus `max-retries` + `send-timeout-ms` rows that were missing). (2) [UNIQUE-TO-B] OutboxPollerWorker:253 — `final int currentRetries = record.getRetries()` read the IN-MEMORY batch-load snapshot of retries; OutboxRecordRepository.markFailureAttempt's JPQL did unguarded `SET o.retries = o.retries + 1` against the DB. A concurrent hot-relay failure between batch fetch and persistFailureOutcome could push DB retries past the snapshot value, leaving the poller to compute `budgetExhausted=false` when the DB already showed `>= maxRetries` — one extra increment past budget. Fix: refactored OutboxRecordRepository.markFailureAttempt to a single atomic JPQL UPDATE with a CASE-WHEN that increments retries AND conditionally transitions the row to FAILED when `retries + 1 >= maxRetries`. The budget is now evaluated INSIDE the UPDATE statement against the database's CURRENT retries value (not against any in-memory snapshot). markFailureAttempt signature now takes maxRetries; piped through OutboxPublishCompletion + OutboxPollerWorker + OutboxPublisher (which gained an OutboxProperties dep so the hot-relay path uses the same atomic semantics). The Java-side `budgetExhausted` decision in persistFailureOutcome is removed entirely; the in-memory `record.getRetries()` is used only as best-effort input for the log line + Span attribute (`pending_retry` vs `pending_to_failed`). Both findings' fixes use the same configured `im2be.outbox.poller.max-retries` value. Verification: - mvn -B -pl outbox-publisher test → 67 tests passed (3 new validation tests: lockAtMostMalformedFailsBinding, lockAtLeastMalformedFailsBinding, lockDurationsOverridePassValidation; defaultsPassValidation extended to assert ShedLock defaults) - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - markFailureAttempt signature changed everywhere — repo, completion, poller, publisher all updated. Test stubs across OutboxPublishCompletionTest, OutboxPollerWorkerTest, OutboxPublisherTest updated with 4-arg matcher signatures. - rowAtMaxRetriesTransitionsToFailed + failedRowsAreNotReSweptByPoller tests rewritten: previous shape asserted poller called markFailed() at budget exhaustion. New shape asserts markFailureAttempt() with maxRetries arg — the atomic JPQL handles the transition. - markFailed() repo + completion methods preserved for direct operator-driven terminal transitions (operator runbook reset path); poller no longer calls markFailed() in the normal failure flow.Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 6 — head
44a18b19cb22, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 agreed minor finding (dead
markFailedpublic API); all prior-round findings confirmed resolved.Summary
Arbitration — Round 6
Memora recall: no prior run history found (both queries returned 0 results). Memora persist attempted but failed due to tag permission errors; run summary could not be stored.
Verification performed:
ReadOutboxPublishCompletion.javalines 120–141 → confirmedmarkFailedexists at line 129 exactly as cited by both A and B.ReadOutboxRecordRepository.javalines 185–204 → confirmedmarkFailedexists at lines 192–203 (B's cite range) / line 199 (method declaration).GrepformarkFailedacross all.javafiles → confirmed zero production callers; all production-path hits are the two method definitions themselves plus a Javadoc reference inOutboxProperties.java:110; test-side hits are exclusively inOutboxPublishCompletionTest(unit-tests of the method itself) and fourverify(completion, never()).markFailed(...)assertions inOutboxPollerWorkerTestat lines 160, 183, 259, 329 — exactly as cited by both reviewers.Reconciliation outcome: 1 agreed finding kept (Rule 1 — both A and B cite the same file, same line, materially-equivalent body). No unique-to-one findings to adjudicate. Severity preserved as
minor.Blast Radius
The diff is self-contained within the
outbox-publishersubmodule. The deadmarkFailedmethods are on an internal completion helper and a Spring Data repository interface — both unexported outside the module's own package. Impact is limited to future contributors who might mis-use the orphaned surface before it is removed.BLAST_SCORE: 3/10
Risk Indicators
OutboxPublishCompletion.markFailureAttempt,OutboxPollerWorker.pollAndPublishPending,OutboxPublisher.tryHotPublishCI status (head
44a18b19cb22)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR]
markFailedis orphaned dead public API — bypasses atomic retry-budget enforcementoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublishCompletion.java:129
OutboxPublishCompletion.markFailed(line 129–140) andOutboxRecordRepository.markFailed(line 199–203) are public methods with zero production callers. The FAILED terminal transition is now handled atomically insidemarkFailureAttempt's JPQLCASE WHEN o.retries + 1 >= :maxRetries THEN FAILEDclause. NeitherOutboxPollerWorkernorOutboxPublisherever invokescompletion.markFailed(…)— this is explicitly confirmed by fourverify(completion, never()).markFailed(…)assertions inOutboxPollerWorkerTest(lines 160, 183, 259, 329).Leaving these methods on the public surface is a footgun: a future contributor who encounters
markFailedmight use it to force-terminate a row directly, silently bypassing the atomic retry counter increment inmarkFailureAttempt. That produces a FAILED row whoseretriescolumn was never bumped — inconsistent state that undermines the operator runbook and any alerting that keys onretries.Recommended fix: remove
OutboxPublishCompletion.markFailed(lines 129–140),OutboxRecordRepository.markFailed(lines 192–203), and the correspondingOutboxPublishCompletionTestcases. If an admin escape-hatch for forced terminal transitions is needed in the future, it should be a distinct, loudly-documented admin-only API that accepts an explicitretries-bump argument so accounting stays consistent.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 6 • 1 finding (1m) • 2026-05-27T01:20:14.613Z → 2026-05-27T01:21:39.664Z • posted-as: pr-reviewer-bot
R6 verdict findings (kept=1): (1) [MINOR] OutboxPublishCompletion.java:129 + OutboxRecordRepository.java:199 — `markFailed` is dead public API after R6's atomic markFailureAttempt refactor. The R6 JPQL UPDATE evaluates the retry budget atomically via CASE-WHEN and transitions to FAILED in the same statement; no production code path calls markFailed anymore. The Spring Data repository method + the @Transactional REQUIRES_NEW helper on the completion class were both unreachable orphaned surface. Fix: removed `OutboxPublishCompletion.markFailed(...)` + `OutboxRecordRepository.markFailed(...)` entirely. Operator-driven FAILED→PENDING reset (the runbook recovery path) uses raw SQL, NOT this API — and that runbook is documented in the JPQL operator-runbook comment that survives in `markFailureAttempt`. Removed the 2 unit tests in OutboxPublishCompletionTest that exercised the removed method, plus the 4 `verify(completion, never()).markFailed(...)` assertions in OutboxPollerWorkerTest that were asserting the negative on a method that no longer exists. Updated OutboxProperties.java's stale Javadoc reference (`{@link OutboxRecordRepository#markFailed}` → markFailureAttempt's atomic CASE-WHEN). Verification: - mvn -B -pl outbox-publisher test → 65 tests passed (down from 67: 2 removed markFailed-specific tests + removed never-verify clutter) - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - Searched for all production callers via `grep -rn "markFailed"`: zero hits outside the now-deleted method definitions + the removed test stubs. The R6 refactor genuinely orphaned the method; removing it tightens the public API surface as the reviewer flagged. - The operator runbook comment in markFailureAttempt's Javadoc preserves the recovery path documentation; consumer services do not lose any actual capability.Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 7 — head
bd90d41ed142, basemain, triggersynchronizeTL;DR: NO_NEW_FINDINGS — No new findings this round.
Summary
[quorum-converged] A=0 = B=0. ## Round 7 review
All 19 prior-round findings are resolved. The diff addresses: async CB accounting (R1), FAILED-status transition via JPQL CASE-WHEN (R1), bounded
.get(timeout,…)(R1),ExecutionExceptionunwrapping (R1), broadExceptioncatch inpersistFailureAttempt(R1), span closed after async ack (R1), pageable-only repo method (R1),status = PENDINGguards onmarkSent/markFailureAttempt(R2),InterruptedExceptionbreaks the batch loop (R2), JSR-303@ValidatedonOutboxProperties(R2), OTel context propagated into the async callback (R2),OutboxAutoConfigurationPackagesRegistrarreplacing@EntityScan(R3),ObjectProvider<MeterRegistry>no-op fallback (R3),isTerminal()now returnstrueforFAILED(R3), explicitCircuitBreakerConfigper properties (R4),markSenthard-codesSENT(R4),lockAtMost/lockAtLeastdocumented + pattern-validated (R5), atomic budget evaluation inside JPQL (R5),markFaileddead API removed (R6).One new blocking finding: the
@EnableJpaRepositoriesannotation left onOutboxAutoConfigurationsilently backs offJpaRepositoriesAutoConfigurationfor all consumer services.CI status (head
bd90d41ed142)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 7 • 0 findings • 2026-05-27T01:24:25.970Z → 2026-05-27T01:31:15.793Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]
R7 verdict contradicted itself: TL;DR said NO_NEW_FINDINGS but the prose surfaced one new "blocking" observation. The observation is real and fleet-fatal — same shape as the R3 @EntityScan clobber, but for Spring Data JPA repositories. The diagnosis: - OutboxAutoConfiguration carried `@EnableJpaRepositories(basePackageClasses = OutboxRecordRepository.class)`. - That annotation imports `JpaRepositoriesRegistrar`, which registers a `JpaRepositoryFactoryBean` + `JpaRepositoryConfigExtension` scoped to ONLY the library's package. - Spring Boot's `JpaRepositoriesAutoConfiguration` is guarded by `@ConditionalOnMissingBean({ JpaRepositoryFactoryBean.class, JpaRepositoryConfigExtension.class })` — the library's registration trips the guard and silently disables the consumer's autoconfig. - Result: every consumer service's own Spring Data repositories (in non-library packages) go unscanned at startup. Fleet-fatal silent breakage, same class as R3. Fix: removed `@EnableJpaRepositories` from `OutboxAutoConfiguration`. The existing `OutboxAutoConfigurationPackagesRegistrar` (R3 fix) registers `com.aim2be.platform.outbox` with `AutoConfigurationPackages` ADDITIVELY. Spring Boot's `JpaRepositoriesAutoConfiguration` reads `AutoConfigurationPackages.get(...)` when it scans for `@Repository` beans, so `OutboxRecordRepository` is picked up alongside the consumer's own repos — without the library having to declare `@EnableJpaRepositories`, without disabling the consumer's autoconfig. Updated the registrar's Javadoc to document both concerns (entity scan + repository scan) — they share the same underlying mechanism (`AutoConfigurationPackages.get(...)`), and library authors should NEVER apply `@EntityScan` OR `@EnableJpaRepositories` on autoconfig classes. Verification: - mvn -B -pl outbox-publisher test → 65 tests passed - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - OutboxRecordRepository.java is unchanged; it remains a Spring Data interface annotated effectively via auto-config scan. - All existing tests + ITs continue to pass because the existing OutboxAutoConfigurationPackagesRegistrarTest already proves the additive package-registration shape works for the consumer-scan path.Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 8 — head
8978c78013e6, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 findings (1 agreed minor @EnableSchedulerLock default-leak, 1 unique-to-A info test coupling); all 19 prior-round findings confirmed resolved.
Summary
Round 8 Arbitration — 2 kept (1 agreed, 1 unique-to-A verified)
Prior findings: All 19 findings from rounds 1–6 confirmed resolved by both reviewers; no re-raises required.
Agreed finding (A+B):
@EnableSchedulerLock(defaultLockAtMostFor = "PT1M")atOutboxAutoConfiguration.java:79confirmed present — both reviewers independently raised it asminor. Kept.Unique-to-A finding:
ITConfig's@SpringBootApplicationatOutboxPublisherIT.java:211— verified:ITConfigis indeed annotated@SpringBootApplication(static inner class in packagecom.aim2be.platform.outbox), andFakeConsumerApplicationis confirmed incom.aim2be.platform.outbox.fakeconsumerannotated@Configuration, so it will be component-scanned byITConfig. Currently harmless (no entities/repos infakeconsumer). Kept asinfo.Memora: No prior run history found (Memora empty for this PR despite round 8); created new entry (id=297) with
review_count=8via fallback path.Blast Radius
The
@EnableSchedulerLockside-effect touches every consuming service that loads the auto-configuration without declaring their own@EnableSchedulerLock. This is an exported auto-configuration surface; the impact is application-wide ShedLock default behavior in every downstream service. The IT finding is test-scope only with zero production blast.BLAST_SCORE: 6/10
Risk Indicators
OutboxAutoConfiguration.outboxCircuitBreaker,OutboxPollerWorker.pollAndPublishPending,OutboxPublisher.tryHotPublishCI status (head
8978c78013e6)Overall: ✗ failure
2 checks: 2 pending
Related PRs
Findings (2)
[MINOR]
@EnableSchedulerLock(defaultLockAtMostFor = "PT1M")on the auto-configuration silently becomes the application-wide ShedLock default for consumersoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:79
The class-level
@EnableSchedulerLock(defaultLockAtMostFor = "PT1M")onOutboxAutoConfigurationregistersPT1Mas the fallbacklockAtMostForfor the entire application context. The library's ownpollAndPublishPending()is unaffected because it always specifies an explicitlockAtMostFor = "${im2be.outbox.poller.lock-at-most:PT1M}". However, any consuming-service@SchedulerLockannotation that omitslockAtMostForwill silently inheritPT1M.If the consumer also declares
@EnableSchedulerLockon their@SpringBootApplication, Spring deduplicates the import and the consumer's attributes win. But a common first-time adoption pattern is to rely solely on the library's auto-configuration — in that casePT1Mbecomes an unconfigurable application-wide default that shortens whatever lock window the consumer intended.The README's configuration-reference table documents
im2be.outbox.poller.lock-at-mostbut does not disclose that loading the auto-configuration activates@EnableSchedulerLockglobally.Recommended fixes (in order of robustness):
@EnableSchedulerLockfromOutboxAutoConfigurationand instead register ShedLock's infrastructure beans programmatically with@ConditionalOnMissingBean, matching the pattern already used forLockProvider. This prevents any class-level default from leaking.@EnableSchedulerLock(defaultLockAtMostFor = "PT1M"); if your service has additional@SchedulerLock-annotated methods, always specifylockAtMostForexplicitly on those annotations, or declare your own@EnableSchedulerLockon@SpringBootApplicationto control the application-wide default."[INFO]
ITConfig's@SpringBootApplicationcomponent-scansFakeConsumerApplicationfrom thefakeconsumersub-packageoutbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPublisherIT.java:211
OutboxPublisherIT.ITConfigis a@SpringBootApplication-annotated static class whose effective package iscom.aim2be.platform.outbox. Spring Boot's default component scan covers all sub-packages — includingcom.aim2be.platform.outbox.fakeconsumer.FakeConsumerApplicationthere is@Configuration-annotated, so it is picked up by the IT context, triggering its@AutoConfigurationPackage(which registerscom.aim2be.platform.outbox.fakeconsumerwithAutoConfigurationPackages) and its@Import(OutboxAutoConfigurationPackagesRegistrar.class)(which re-registerscom.aim2be.platform.outbox— idempotent).Currently harmless because
fakeconsumercontains no entities or repositories, but it creates unintended coupling:FakeConsumerApplicationwas authored exclusively forOutboxAutoConfigurationPackagesRegistrarTestand its presence in the IT context is a side-effect of package layout, not design.Concrete fix (either):
FakeConsumerApplicationto a package that is not a sub-package of the IT scan root (e.g.com.aim2be.platform.outbox.test.fakeconsumer) and add an explicit exclusion filter inITConfig.ITConfig's scan:@SpringBootApplication(scanBasePackages = "com.aim2be.platform.outbox")plus an explicitexcludeFiltersthat excludesFakeConsumerApplication.Appropriate for a follow-on test-hygiene PR; not a blocker.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 8 • 2 findings (1m/1i) • 2026-05-27T01:42:21.369Z → 2026-05-27T01:43:56.417Z • posted-as: pr-reviewer-bot
R8 verdict findings (kept=2): (1) [MINOR, agreed A+B] OutboxAutoConfiguration.java:79 — `@EnableSchedulerLock(defaultLockAtMostFor = "PT1M")` hard-coded the PT1M context-wide default. Every consumer that loads the autoconfig without declaring their own `@EnableSchedulerLock` would have ShedLock apply PT1M to their `@SchedulerLock`-annotated methods that omit `lockAtMostFor`. The library was leaking its opinion into downstream services. Fix: ShedLock 6.x requires `defaultLockAtMostFor` (the annotation declares it without a Java default value), so we cannot simply drop it. Switched to Spring's annotation-value property-placeholder: `defaultLockAtMostFor = "${im2be.outbox.scheduler-lock.default-at-most:PT1M}"`. The library still has a sensible fallback (PT1M) but consumers can override `im2be.outbox.scheduler-lock.default-at-most` to change it. The library's own `@SchedulerLock` on `pollAndPublishPending()` sets `lockAtMostFor` + `lockAtLeastFor` explicitly via separate property placeholders, so the default never affects the poller itself. Documented the property + ShedLock interaction in OutboxAutoConfiguration's class Javadoc and the README config table. (2) [INFO, unique-to-A] OutboxPublisherIT.java:211 — `ITConfig`'s `@SpringBootApplication` component-scans `com.aim2be.platform.outbox`, which includes the `fakeconsumer` subpackage where `FakeConsumerApplication` lives. Currently harmless (the IT context has no entities/repos that depend on the fakeconsumer's wiring), but a future-leak. Fix: gated `FakeConsumerApplication` with `@ConditionalOnProperty(prefix = "im2be.outbox.test.fakeconsumer", name = "enabled", havingValue = "true")`. The IT's broad component scan still TOUCHES the class but the conditional keeps it out of the bean factory unless explicitly enabled. The OutboxAutoConfigurationPackagesRegistrarTest sets `im2be.outbox.test.fakeconsumer.enabled=true` on its ApplicationContextRunner so the additivity test still works. Verification: - mvn -B -pl outbox-publisher test → 65 tests passed - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - The placeholder-driven `defaultLockAtMostFor` value is resolved by Spring's annotation post-processor at @Configuration parsing time; the only consumer-visible behavior change is that the property is now respected. - `@ConditionalOnProperty` on `FakeConsumerApplication` is a no-op when the property is absent (default) — Spring sees the class metadata, evaluates the condition false, skips bean registration. The IT context is unaffected. The registrar test explicitly enables the property.Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 9 — head
8383bd152d07, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 findings (1 agreed minor, 1 unique-to-A info verified); 21 prior-round findings remain resolved; no blocking issues.
Summary
Arbitration — Round 9
Memora recall: No prior run history found (tags allowlist prevented write; both create and absorb paths blocked by permission/tag constraints).
Finding reconciliation:
Finding 1 — AGREED (A+B), kept: Both reviewers independently flag that
im2be.outbox.scheduler-lock.default-at-mostis consumed via annotation placeholder atOutboxAutoConfiguration.java:94but has no corresponding field inOutboxProperties. Verified withRead+Grep:OutboxProperties.javacontains zero matches forschedulerLock/scheduler-lock/SchedulerLock. Line 94–95 confirms@EnableSchedulerLock(defaultLockAtMostFor = "${im2be.outbox.scheduler-lock.default-at-most:PT1M}")with no JSR-303 backing. The parallelpoller.lock-at-mostandpoller.lock-at-leasteach have@Pattern-validated fields inOutboxProperties.Poller— the gap is real. Kept as minor.Finding 2 — UNIQUE-TO-A, verified, kept: A flags that
markSent(line 99) andmarkFailureAttempt(line 150) JPQL bulk UPDATEs never incremento.recordVersion. Verified: the SET clause ofmarkSent(lines 101–106) setsstatus,sentAt, andlastErroronly;markFailureAttempt(lines 152–161) setsretries,lastError, andstatusonly — neither includeso.recordVersion = o.recordVersion + 1.@Version private long recordVersionis confirmed present atOutboxRecord.java:136–138. Hibernate does not auto-increment@Versioncolumns in JPQL bulk UPDATE paths. No active runtime defect; theAND o.status = PENDINGguard provides safe idempotency for the library's own write paths. Kept as info (deferred).Result: 1 agreed finding kept, 1 unique-to-A verified and kept, 0 dropped.
Blast Radius
The diff touches a single submodule (
outbox-publisher) but that module ships a Spring Boot autoconfiguration that is imported by all 7 aim2be JPA services. Any misconfiguration of the scheduler-lock property surface propagates silently to every consuming service at startup. The JPQL version-column gap is scoped to theoutbox_recordstable writes only.BLAST_SCORE: 4/10
Risk Indicators
markSent,markFailureAttempt,tryHotPublish,persistFailureAttempt,OutboxPublishCompletion.handleAsyncAckCI status (head
8383bd152d07)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MINOR]
im2be.outbox.scheduler-lock.default-at-mosthas noOutboxPropertiesbinding — no JSR-303 validation and no IDE autocompleteoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:94
Line 94–95 reads the property via annotation placeholder:
OutboxProperties(bound via@ConfigurationProperties(prefix = "im2be.outbox")) has no field forscheduler-lock.default-at-most— confirmed by grep: zero matches for anyschedulerLock/SchedulerLock/scheduler-locksymbol inOutboxProperties.java. By contrast, the parallelim2be.outbox.poller.lock-at-mostandim2be.outbox.poller.lock-at-leasteach have a@Pattern(regexp = "^PT[0-9]+...")-validated field inOutboxProperties.Poller, so an invalid value there produces a cleanBindValidationExceptionat context startup. An invalidscheduler-lock.default-at-mostinstead silently passes binding and fails late inside ShedLock'sDuration.parse()with an opaqueDateTimeParseException. The property is also absent fromOutboxPropertiesValidationTestand will not appear inspring-configuration-metadata.jsonfor IDE autocomplete.Fix: add a
SchedulerLocknested class (or top-level field) toOutboxPropertieswith the same ISO-8601@PatternaslockAtMost, default"PT1M", and add aschedulerLockDefaultAtMostMalformedFailsBindingtest alongside the existinglockAtMostMalformedFailsBindingtest.[INFO] JPQL bulk UPDATEs in
markSent/markFailureAttemptdo not incrementrecord_version—@Versionoptimistic lock non-functional for all post-INSERT status transitionsoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxRecordRepository.java:99
Both
markSent(lines 99–111) andmarkFailureAttempt(lines 150–167) are Hibernate JPQL bulk UPDATE statements. Hibernate executes bulk JPQL as a native SQLUPDATEand does not automatically increment@Version-annotated columns in that path. The SET clause ofmarkSentcoversstatus,sentAt, andlastErroronly;markFailureAttemptcoversretries,lastError, andstatusonly — neither includeso.recordVersion = o.recordVersion + 1.OutboxRecord.recordVersionis annotated@Version(line 136) and required by ADR-0001 §1.4EntityVersionParity, but it stays at its INSERT-time value (0) for the row's entire lifecycle after the firstsave().The library's own concurrent-write safety is intact — the
AND o.status = PENDINGpredicate provides idempotency for all current write paths. The risk is latent: a consuming service (or future contributor) that loads anOutboxRecord, mutates a field, and callsrepository.save()will findrecord_version = 0in both the entity snapshot and the DB, so the optimistic-lock check passes and the entity-level save silently overwrites whatever status the JPQL paths had written.Deferred — no active runtime defect. Remediation options: (1) add
o.recordVersion = o.recordVersion + 1to both JPQL SET clauses; (2) makeOutboxRecordsetters package-private to block consuming-service entity-level status writes; (3) document the constraint in@ModifyingJavadoc on both methods.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 9 • 2 findings (1m/1i) • 2026-05-27T01:52:59.993Z → 2026-05-27T01:55:02.972Z • posted-as: pr-reviewer-bot
R9 verdict findings (kept=2): (1) [MINOR, agreed A+B] OutboxAutoConfiguration:94 + OutboxProperties — the R9 fix that turned `defaultLockAtMostFor` into a property-placeholder (`${im2be.outbox.scheduler-lock.default-at-most:PT1M}`) closed the leak but left the property undocumented + unvalidated. The parallel `poller.lock-at-most` and `poller.lock-at-least` both have `@Pattern`-validated typed fields in OutboxProperties.Poller; this one didn't, breaking symmetry. Fix: added nested `OutboxProperties.SchedulerLock` class with `String defaultAtMost` field (default "PT1M") + @Pattern ISO-8601 validation. Wired into the parent OutboxProperties with @Valid + a `getSchedulerLock()` getter. Updated the class-level property-reference Javadoc to include the new knob. Added 2 new validation tests (`schedulerLockDefaultAtMostMalformedFailsBinding` + `schedulerLockDefaultAtMostOverridePassValidation`). (2) [INFO deferred, unique-to-A] OutboxRecordRepository:101-106 + 152-161 — JPQL bulk UPDATEs in `markSent` + `markFailureAttempt` don't increment `o.recordVersion` because Hibernate only auto-bumps @Version on entity-managed updates (load → modify → save), not on JPQL bulk UPDATE paths. Reviewer marks this no-active-defect because the `AND o.status = PENDING` status-guard provides equivalent concurrency safety for the PENDING → terminal transitions the library cares about. Fix: added explicit Javadoc on the `@Version private long recordVersion` field in OutboxRecord explaining the JPQL bulk-UPDATE bypass + the status-guard concurrency-safety rationale. The @Version field is retained to satisfy ADR-0001 §1.4 (EntityVersionParity ArchUnit rule) and to provide optimistic locking for any future entity-managed write that consumer code might add — but the library's own bulk-UPDATE paths intentionally bypass it. Verification: - mvn -B -pl outbox-publisher test → 67 tests passed (2 new validation tests) - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - The new SchedulerLock property class is purely additive — no callsite changes are required since the @EnableSchedulerLock annotation still reads the raw property placeholder. The typed field is documentation + binding-time validation only. - @Version field documentation change is comment-only; no behavioral impact.Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 10 — head
5027e5d49d10, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 minor verified unique-to-A finding (markSent exception silently swallowed in whenComplete); all 23 prior-round findings confirmed closed by both reviewers.
Summary
Round 10 arbitration
Reconciliation: A=1 minor, B=0. Unique-to-A finding verified via
ReadatOutboxPublisher.javalines 306–362.Verification of A's finding (line 320): The
tryblock at line 316 covers both the success path (includingcompletion.markSent()at line 320) and the failure path, with only afinallyclause (lines 332–334) and nocatch. Ifcompletion.markSent()throws, the exception bubbles through thefinally(span.end()fires), exitshandleAsyncAck, and is re-thrown inside thewhenCompletelambda — whereCompletableFuturesilently discards it. This asymmetry withpersistFailureAttempt(lines 350–362), which wraps the equivalentcompletion.markFailureAttempt()call in an explicittry { … } catch (Exception e) { LOG.error(…) }, is confirmed real. Finding kept as minor.Memora persistence: create attempted; permission denied — best-effort record-keeping not available this run.
Kept 1 agreed/verified finding (unique-to-A, verified). Dropped 0. All 23 prior-round findings confirmed closed by both reviewers.
Blast Radius
The diff introduces a net-new Spring Boot autoconfiguration module (outbox-publisher) that exports shared JPA entities, a transactional publisher, and a ShedLock-coordinated poller to all 7 aim2be JPA services. The exported autoconfiguration surface and
OutboxRecordentity schema affect every consuming service that enablesim2be.outbox.enabled=true, but the module itself is additive and does not modify any existing shared contracts or migration files.BLAST_SCORE: 5/10
Risk Indicators
OutboxPublisher.handleAsyncAck,OutboxPublisher.persistFailureAttempt,OutboxPublishCompletion.markSent,OutboxPublishCompletion.markFailureAttempt,OutboxPollerWorker.pollAndPublishPending,OutboxAutoConfiguration.outboxCircuitBreakerCI status (head
5027e5d49d10)Overall: ✗ failure
2 checks: 2 pending
Related PRs
Findings (1)
[MINOR]
completion.markSent()exception insidewhenCompletesilently swallowed — no log, unlikepersistFailureAttemptoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:320
Verified at lines 306–362.
handleAsyncAckopens atryblock at line 316 with only afinallyclause (lines 332–334,span.end()); there is nocatch. Ifcompletion.markSent()at line 320 throws (e.g.CannotCreateTransactionException, anyDataAccessException, or another unchecked exception from theREQUIRES_NEWtransaction), the exception propagates out ofhandleAsyncAck, is re-thrown inside thewhenCompletelambda, and is silently discarded byCompletableFuturemachinery — no log entry, no metric, no alert.Contrast with
persistFailureAttempt(lines 350–362), which wraps the equivalentcompletion.markFailureAttempt()call in an explicittry { … } catch (Exception e) { LOG.error(…) }block for exactly this reason.The poller provides eventual correctness (the PENDING row is picked up and marked SENT on the next tick), but operators have zero visibility into silent hot-relay bookkeeping failures. Add a defensive wrapper analogous to
persistFailureAttempt:This mirrors the failure path's existing error-swallowing contract while restoring operator observability.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 10 • 1 finding (1m) • 2026-05-27T02:03:00.895Z → 2026-05-27T02:04:32.287Z • posted-as: pr-reviewer-bot
R10 verdict findings (kept=1): (1) [MINOR, unique-to-A verified] OutboxPublisher.handleAsyncAck:320 — `completion.markSent(...)` was called directly inside the try/finally with NO catch. An exception from `markSent` (CannotCreateTransactionException, DataAccessException, etc.) would bubble through the `finally { span.end(); }`, exit handleAsyncAck, and then be silently discarded by `CompletableFuture.whenComplete` — the hot-relay would have NO visibility into markSent failures. Asymmetric with the failure path (`persistFailureAttempt`) which already wraps `completion.markFailureAttempt(...)` in try/catch(Exception) + LOG.error. Fix: extracted `persistMarkSent(aggregateId, eventId)` helper that mirrors `persistFailureAttempt`'s shape — calls `completion.markSent(...)` inside try { … } catch (Exception persistError) { LOG.error(...) }. handleAsyncAck's success branch now calls `persistMarkSent(...)` instead of directly invoking completion.markSent. The catch broadens to Exception (not just DataAccessException) so sibling transaction-construction exceptions (CannotCreateTransactionException extends TransactionException, NOT DataAccessException) are also swallowed-with-log. Verification: - mvn -B -pl outbox-publisher test → 67 tests passed - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - Behavior change is strictly additive: success-path markSent failures now produce an ERROR log line where previously they were silent. No existing semantics are altered. - handleAsyncAck's `finally { span.end(); }` continues to fire regardless of the markSent outcome (the new try{} is inside persistMarkSent's own scope). - The success branch's metrics.recordPublishLatency(...) call still runs after persistMarkSent returns — markSent failures no longer skip the latency observation (a minor improvement: the publish DID succeed at the Kafka level even if the DB write failed).Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 11 — head
904b338de73d, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 agreed finding (verified at HEAD), 0 dropped; sole remaining issue is a minor retry-budget mischarge in
tryColdPublishwhenmarkSentthrows after a successful Kafka send.Summary
Round 11 Arbitration
Memora context: No prior run history found (first arbitration stored as memory ID 299). No reusable submodule patterns on record.
Reconciliation: Both A and B raised exactly one finding each, pointing at the same file (
OutboxPollerWorker.java), the same line (214), and the same root cause:completion.markSent()andmetrics.recordPublishLatency()at lines 214–217 are co-located inside the Kafka-sendtryblock (lines 211–232). When the Kafka send at lines 212–213 succeeds butcompletion.markSent()subsequently throws (e.g.CannotCreateTransactionException), the exception falls through to the genericcatch (Exception sendError)at line 229, which callspersistFailureOutcome(record, sendError)at line 230 — spuriously incrementingretrieson a row whose message was already delivered.Verification at HEAD confirmed:
OutboxPollerWorker.javamatch the structure both reviewers described exactly.completion.markSent()at line 214 andmetrics.recordPublishLatency()at lines 215–217 are inside the sametryblock as the Kafka.get()call.catch (Exception sendError)at line 229 →persistFailureOutcome(record, sendError)at line 230 covers all of these.persistMarkSent()helper atOutboxPublisher.java:350) was also verified: it has a dedicatedcatch (Exception persistError)that logs without callingpersistFailureOutcome, so the hot path correctly isolates DB-layer failures from Kafka-delivery failures.Outcome: 1 agreed finding kept; 0 unique-to-one findings to evaluate.
Blast Radius
The defect is confined to
OutboxPollerWorker.tryColdPublish, a single package-private method in one class. It affects all outbox publishers using the cold-relay path, but the impact is limited to partial-DB-connectivity edge cases wheremarkSentfails after a successful Kafka ack. No shared interfaces, no migrations, no exported API contracts are affected.BLAST_SCORE: 3/10
Risk Indicators
tryColdPublish,persistFailureOutcome,completion.markSent,completion.markFailureAttemptCI status (head
904b338de73d)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR]
completion.markSent()andmetricscalls intryColdPublishshare the Kafka-send try block — DB/metrics exceptions incorrectly charge the retry budget on delivered rowsoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:214
In
tryColdPublish(lines 211–232),kafkaTemplate.send(...).get(sendTimeoutMs, MILLISECONDS)(lines 212–213),completion.markSent(...)(line 214), andmetrics.recordPublishLatency(...)(lines 215–217) are all inside a singletryblock. The catch ladder at lines 219–231 has specific handlers forInterruptedException,TimeoutException, andExecutionException, but the terminalcatch (Exception sendError)at line 229 captures any remaining exception — includingDataAccessExceptionsubtypes thrown bymarkSentwhen the DB connection is momentarily unavailable after a successful Kafka ack.When that path is taken,
persistFailureOutcome(record, sendError)at line 230 is called, which callscompletion.markFailureAttempt()and incrementsretrieson a row whose message was already delivered to Kafka. Under sustained partial DB connectivity (markSent fails every tick, markFailureAttempt intermittently succeeds), the retry counter creeps upward untilretries ≥ maxRetriesand the CASE-WHEN inmarkFailureAttempttransitions the row toFAILED— despite the message having been publishedmaxRetriestimes.The hot-relay path received this exact fix in round 10:
OutboxPublisher.persistMarkSent()(line 350) wrapscompletion.markSent()in a dedicatedcatch (Exception persistError)that logs and returns normally, never callingpersistFailureOutcome. The cold poller needs the same split:This mirrors the hot-path fix exactly and prevents false-positive
FAILEDtransitions caused by transient DB connectivity issues after successful Kafka delivery.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 11 • 1 finding (1m) • 2026-05-27T02:12:25.145Z → 2026-05-27T02:14:08.653Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 12 — head
c200f6362fa6, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 unique-to-one findings (both minor, both verified); no prior-round regressions; 25 historical findings confirmed resolved at HEAD.
Summary
Memora persist attempted (MCP tag-allowlist error on all attempts — server-side constraint). Reconciliation: both findings are unique-to-one; both verified against HEAD.
Finding A (verified):
OutboxPollerWorkerTest.java—persistFailureSwallowsRepositoryExceptionat line 259 confirmed; scanned through line 363 and found NO test covering the symmetric success path (markSentthrows afterkafkaTemplate.send().get()succeeds). The R11 production fix atOutboxPollerWorker.java:234–257(separate try/catch blocks) is therefore unguarded by a regression test. KEPT.Finding B (verified): All 5 cited call sites confirmed at HEAD —
OutboxPublisher.java:354and:383passpersistError.getMessage()(String);OutboxPollerWorker.java:244,:255,:323also pass.getMessage()as the final SLF4J format argument. SLF4J/Logback only auto-appends the stack trace when the last arg is theThrowableinstance; using a String discards it entirely. KEPT.Kept 2 (A=1 unique + B=1 unique), dropped 0. Both minor. Verdict: CONDITIONAL_APPROVE.
Blast Radius
The diff adds the full outbox-publisher library module — an exported auto-configuration surface consumed by all 7 aim2be JPA services. Changes to
OutboxPublisher,OutboxPollerWorker, andOutboxAutoConfigurationaffect every downstream consumer that setsim2be.outbox.enabled=true. The two remaining findings are minor (missing test + logging quality) and do not affect runtime correctness of the exported API.BLAST_SCORE: 5/10
Risk Indicators
markSent,markFailureAttempt,persistMarkSent,persistFailureAttempt,persistFailureOutcome,tryColdPublishCI status (head
c200f6362fa6)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MINOR] R11 fix has no regression-guard test —
tryColdPublishmust returntrueand NOT callmarkFailureAttemptwhenmarkSentthrows after a successful Kafka.get()outbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPollerWorkerTest.java:280
The R11 production fix (separating
markSent+metricsinto their owntry/catchblocks after the Kafka send intryColdPublish, lines 234–257 ofOutboxPollerWorker.java) prevents amarkSentDB exception from being routed topersistFailureOutcome, which would incorrectly decrement the row's retry budget even though the Kafka message was already delivered. This invariant is untested.OutboxPollerWorkerTestalready haspersistFailureSwallowsRepositoryException(verified at lines 259–280) covering the analogous failure path (i.e.,markFailureAttemptthrowing returnsfalsewithout propagation). The missing symmetric test for the success path:Without this test, a future refactor that accidentally re-merges the post-send blocks back into the Kafka-send
trywould silently re-introduce the R11 regression — a row delivered to Kafka would have its retry budget decremented, eventually transitioning toFAILEDdespite successful delivery.[MINOR] Five ERROR/WARN catch blocks discard stack traces — pass
Throwableas the final SLF4J argument, notgetMessage()outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:354
All five
catch (Exception …)blocks that guard critical post-send persistence paths passexception.getMessage()(aString) as the final SLF4J format argument, permanently discarding stack traces. Verified at HEAD:OutboxPublisher.java:354(persistMarkSent):persistError.getMessage()OutboxPublisher.java:383(persistFailureAttempt):persistError.getMessage()OutboxPollerWorker.java:244(tryColdPublishmarkSent catch):persistError.getMessage()OutboxPollerWorker.java:255(tryColdPublishmetrics catch):metricsError.getMessage()OutboxPollerWorker.java:323(persistFailureOutcomecatch):persistError.getMessage()SLF4J/Logback appends the full stack trace automatically only when the last argument passed to the logger is the
Throwableitself. UsinggetMessage()means that when these paths fire in production — a connection reset duringmarkSent, a JPQL mapping failure, a Micrometer registry error — the only diagnostic is a one-line message with no frame information.Fix: append the exception as a trailing argument (SLF4J treats a trailing
Throwablespecially even without a corresponding{}placeholder):Apply the same correction at
OutboxPublisher.java:383,OutboxPollerWorker.java:244,OutboxPollerWorker.java:255, andOutboxPollerWorker.java:323.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 12 • 2 findings (2m) • 2026-05-27T02:22:05.364Z → 2026-05-27T02:24:03.898Z • posted-as: pr-reviewer-bot
R12 verdict findings (kept=2): (1) [MINOR, unique-to-A verified] OutboxPollerWorkerTest — the R12 production fix that splits tryColdPublish into Kafka-send (Phase 1) + post-send markSent+metrics (Phase 2) had NO regression test guarding the "Kafka send succeeds but markSent throws" path. A future revert or refactor could silently re-introduce the spurious retry-bump. Fix: added `markSentFailureAfterSuccessfulSendDoesNotBumpRetries` test in OutboxPollerWorkerTest. Stubs kafkaTemplate.send to return a completed-OK future, stubs completion.markSent to throw CannotCreateTransactionException, asserts tryColdPublish returns true (Kafka ack stands) AND verify(completion, never()).markFailureAttempt(...) (no retries bump). (2) [MINOR, unique-to-B verified] All persist-error LOG.{error,warn} sites passed `e.getMessage()` (String) as the last arg — SLF4J/Logback only auto-append the stack trace when the LAST argument is the Throwable instance. Using a String discards the stack trace entirely. Affected sites: - OutboxPublisher.java:331 — kafka_error in handleAsyncAck - OutboxPublisher.java:356 — persistMarkSent catch - OutboxPublisher.java:383 — persistFailureAttempt catch - OutboxPollerWorker.java:244 — markSent post-send catch (R12 fix) - OutboxPollerWorker.java:255 — recordPublishLatency post-send catch (R12 fix) - OutboxPollerWorker.java:323 — persistFailureOutcome outer catch Fix: replaced `{}` placeholder + `e.getMessage()` arg with the bare Throwable instance at every site. SLF4J/Logback now print the full stack trace inline with the log line. Each log message body still includes the structured context (topic, eventId, aggregateId). Verification: - mvn -B -pl outbox-publisher test → 68 tests passed (1 new regression test) - mvn -B -pl outbox-publisher -am install -DskipITs → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - Log-format change is observable in operator logs: stack traces now print where previously only single-line messages did. This is a strict improvement for debugging. - The new test exercises the exact Phase-2 path the R12 production fix introduced; it'd be the first to fail if Phase 2 regressed.hib-pr-reviewer review — PR #4 (affinity-intelligence-rework/im2be-platform-libs)
Round 13 — head
4775abfc2e3c, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 minor unique-to-A finding (verified at line 278); B returned no structured findings; no blocking issues.
Summary
Round 13 reconciliation — PR #4 (outbox-publisher full impl)
Memora context: No prior run memories found (tags blocked); created new entry (id=302) with round-13 summary.
Reviewer B: Returned
findings: []in their JSON (prose summary was a best-effort salvage from a truncated output). No structured findings to evaluate.Reviewer A: 1 minor finding —
metrics.recordFailureunguarded inpersistFailureOutcome. This is unique-to-A and required verification.Verification performed (Rule 2): Read
OutboxPollerWorker.javalines 250–328 and 100–170.metrics.recordFailure(record.getTopic())before thetryblock (which begins at line 292) that wrapscompletion.markFailureAttempt. If a metrics implementation throws fromcounter.increment(),markFailureAttemptis never called.tryColdPublish(line 207) callspersistFailureOutcomeinsidecatchblocks (lines 219, 222, 225). The per-row loop inpollAndPublishPending(lines 144–165) only catchesInterruptedException— a non-interrupt exception escapingtryColdPublishpropagates out of theforloop, past the outertry/finally, and aborts the entire tick.metrics.recordPublishLatency(lines 248–258) is wrapped in its own try/catch with aLOG.warn. The failure path has no equivalent guard.Finding kept (1 of 1). All R1–R12 findings remain closed per the verified resolution table in Reviewer A's summary.
Blast Radius
The affected code path is confined to the cold-relay poller worker's failure-outcome persistence helper. Impact is limited to the
outbox-publishermodule's internal retry accounting; the hot-relay path and all other modules are unaffected.BLAST_SCORE: 3/10
CI status (head
4775abfc2e3c)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR]
metrics.recordFailureis unguarded inpersistFailureOutcome— tick aborts if it throws, skippingmarkFailureAttemptoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPollerWorker.java:278
In
persistFailureOutcome,metrics.recordFailure(record.getTopic())is called at line 278, before thetryblock (line 292) that protectscompletion.markFailureAttempt. If a customMeterRegistryimplementation throws fromcounter.increment(), the method unwinds without persisting the retry-count increment.The exception then propagates through
tryColdPublish's catch blocks (lines 219/222/225), up to the per-row loop inpollAndPublishPending. That loop only catchesInterruptedException(lines 151–164); any other exception escapes the loop and aborts the entire tick, silently skipping all remaining batch rows.Contrast with the success path (lines 248–258), where
metrics.recordPublishLatencyis symmetrically wrapped:Fix: Move
metrics.recordFailure(record.getTopic())inside the existingtryblock before themarkFailureAttemptcall, or wrap it in its own try/catch consistent with the success-path pattern.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 13 • 1 finding (1m) • 2026-05-27T02:33:45.268Z → 2026-05-27T02:35:13.599Z • posted-as: pr-reviewer-bot