feat(outbox): PR-PLATFORM-1-FOLLOWUP-EVENTID — Function<UUID,byte[]> publish overload for event_id lockstep #6
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/platform-1-followup-eventid-overload"
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?
TL;DR
APPROVEcandidate — adds theOutboxPublisher.publish(String, Function<UUID,byte[]>, int, UUID)deferred-serialisation overload so the outbox row'sevent_idand the payload's embeddedevent_idstay in lockstep (fixes Memora #3555 / the R1 finding on social-login PR #10). Implements the independently-reviewed design (Memora #3556, APPROVE_WITH_REFINEMENTS @ 88%). All 6 hardening refinements applied. 80 unit + 4 IT tests pass, zero warnings.Summary
The existing
publish(String, byte[], int, UUID)generates the outbox-rowevent_idinternally — callers can't make their payload's embeddedevent_idmatch, so Kafka↔outbox correlation is broken (operators can't trace a Kafka message back to its outbox row for retry/DLQ). The AFTER_COMMIT relay closes over the original byte[], so post-publish UPDATEs don't help; all caller-side workarounds are blocked (Memora #3555).The new overload hands the outbox-issued UUIDv7 to a
Function<UUID, byte[]>serialiser, which embeds it in the payload before the library captures the bytes. Both row + payload then agree.2 commits: (1) production — overload + dual defensive-clone +
@Deprecatedbyte[] overload + 6 Javadoc contract claims + version bump 1.0.0→1.1.0-SNAPSHOT; (2) tests — 12 unit + 1 IT.Design note: extracted a shared private
persistAndRegisterRelay(...)helper so both overloads delegate after their defensive clone — eliminates flow divergence (improvement over the review's verbatim "duplicate the flow" suggestion).Findings
R0 (initial open). Pre-emptive observations:
OutboxPublisher.java@Deprecated(since="1.1", forRemoval=false). Retained for opaque already-encoded payloads with no event_id field. One production caller (social-loginOAuthUserInfoProvider) migrates in its R3; library-internal byte[]-overload tests carry method-scoped@SuppressWarnings("deprecation")with justification.pom.xml+ 5 modules@Deprecated+ rule-62 (zero-warnings) before tagging, to confirm no dependent service's build breaks on a deprecation warning.OutboxPublisher.javaArrays.copyOfso neither path is vulnerable to caller buffer mutation. T12 regression-proves it.Verdict
APPROVEcandidate. Implements the reviewed design in full; no deviations except thepersistAndRegisterRelayDRY helper (strict improvement). The IT lockstep test is the load-bearing safety net.Verification
Test matrix (per Memora #3556 §4): T1 outside-TX-guard-before-fn / T2 row+eventId lockstep / T3 invoked-once / T4 fn-before-save (InOrder) / T5 throw-propagates / T6 null→NPE / T7 empty→IAE / T8 UUIDv7 / T9 return==fn-arg / T10 row==return / T11 AFTER_COMMIT bytes + defensive-clone / T12 byte[]-overload clone regression / IT
publishFunctionalLockstepEventIdMatchesPayloadField(end-to-end payload.event_id == row.event_id == return, real PG + EmbeddedKafka).Related
pr-review-toolkit:code-reviewersubstitutedHaloService.java:142same pattern)@Transactional-must-be-public rule (Memora #3553 — fired twice in Wave A)Footer: im2be-platform-libs • PR-PLATFORM-1-FOLLOWUP-EVENTID • R0 (initial) • tally
2 commits / +521 lines / 13 new tests / 0 warnings / v1.1.0-SNAPSHOT• 2026-05-28Show previous round
hib-pr-reviewer review — PR #6 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
ec15a216ba0c, basemain, triggeropenedTL;DR: BLOCKED — 1 build-breaking unused import (Propagation, Xlint:all + failOnWarning), 1 minor validation gap in the deprecated overload, 2 info-level observations; 0 findings dropped.
Summary
Arbitration — Round 1
All 4 findings are unique-to-one-reviewer; none are agreed. Each was verified against the HEAD checkout before a keep/drop decision.
B-1 (blocking) KEPT —
import org.springframework.transaction.annotation.Propagationat line 23 is confirmed present. Grep confirms its only usage is{@link Propagation#REQUIRES_NEW REQUIRES_NEW}inside a Javadoc comment at line 240; that is not a live bytecode reference. Parentpom.xmllines 100–103 confirm<failOnWarning>true</failOnWarning>+-Xlint:all. The-Xlint:-processingcarve-out does not cover[imports], so javac will emit an unused-import warning thatfailOnWarningpromotes to a compile error. This is build-blocking.A-1 (minor) KEPT — Lines 183–185 of OutboxPublisher.java confirm the deprecated
byte[]overload performs only a null-check onpayload; nopayload.length == 0guard. Lines 296–298 confirm the newFunctionoverload does guard. The asymmetry is real.A-2 (info) KEPT — IT lines 224–227 fetch
rowEventIdviafindById(new OutboxRecord.PK(aggregateId, returned));returnedIS the eventId component of the PK, so by constructionrowEventId.toString()≡returned.toString(). The assertion at 241–242 is therefore logically identical to 243–245. Reviewer B described the extra round-trip as "intentional triple-assertion value" but did not rebut the structural identity — the DB call isolates no distinct failure mode. Kept as info.B-2 (info) KEPT —
@Deprecated(since = "1.1", forRemoval = false)at line 176 confirmed; parent POM version confirmed as1.1.0-SNAPSHOT. Informational inconsistency.Summary: 1 blocking + 1 minor + 2 info findings kept; 0 dropped.
Blast Radius
The diff spans 9 files across 5 Maven modules but substantive logic changes are confined to
outbox-publisher/src/main/java/.../OutboxPublisher.java(+152/-4) and its test files. The new exportedpublish(String, Function<UUID,byte[]>, int, UUID)overload extends the module's public API surface. Other module changes are version-bump-only (pom.xml coordinates).BLAST_SCORE: 3/10
Risk Indicators
publish,persistAndRegisterRelayCI status (head
ec15a216ba0c)Overall: ✗ failure
2 checks: 2 pending
Related PRs
Findings (3)
[BLOCKING] Unused
Propagationimport will fail the build under Rule 62 (failOnWarning=true+-Xlint:all)outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:23
Grep-confirmed:
import org.springframework.transaction.annotation.Propagation;(line 23) is used only inside a{@link}Javadoc tag at line 240 ({@link Propagation#REQUIRES_NEW REQUIRES_NEW}). Javadoc comments are not compiled to bytecode — javac does not count them as live references. The parentpom.xml(lines 100–103) enforces<failOnWarning>true</failOnWarning>and-Xlint:all(with only-Xlint:-processingexcluded). The[imports]lint category is not exempted; an unused-import warning will be promoted to a compile error.Fix — drop the import and use the fully-qualified name in the Javadoc:
No other usage of
Propagationexists in the file.[INFO] Redundant
findByIdround-trip forrowEventId;sentAtcheck absent after SENToutbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPublisherIT.java:224
Lines 224–227 fetch
rowEventIdviarepository.findById(new OutboxRecord.PK(aggregateId, returned)). Becausereturnedis the eventId component of the composite PK, the retrieved record'seventIdis structurally guaranteed to equalreturned. Consequently the assertion at lines 241–242 (payloadEventId.isEqualTo(rowEventId.toString())) is logically identical to the assertion at lines 243–245 (payloadEventId.isEqualTo(returned.toString())); the extra DB call isolates no distinct failure mode.The sibling
happyPathPublishCommitsAndKafkaReceivesIT assertsrow.getSentAt().isNotNull()after awaiting SENT; the new lockstep test skips this. AddingassertThat(row.getSentAt()).isNotNull()inside theuntilAsserted()lambda at line 217 would make the two ITs consistent in post-SENT validation and would replace the extra round-trip with a meaningful new assertion.[INFO]
@Deprecated(since = "1.1")does not match the artifact version bumped to1.1.0-SNAPSHOToutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:176
Line 176:
@Deprecated(since = "1.1", forRemoval = false). All fivepom.xmlfiles (parent + four submodules) bump to1.1.0-SNAPSHOT. Thesincevalue"1.1"is informational only (IDE tooltips,javapoutput) and does not affect compilation or runtime, but consumers and static-analysis tools that parse it will see a mismatch against the artifact coordinate1.1.0.Consider:
Verdict
BLOCKED
hib-pr-reviewer • round 1 • 3 findings (1b/2i) • 2026-05-28T06:19:24.887Z → 2026-05-28T06:21:11.465Z • posted-as: pr-reviewer-bot
R2 verdict findings (kept=3; B-1 mechanism reframed after verification): (1) BLOCKING B-1 OutboxPublisher.java:23 — "unused Propagation import will fail the build". Verified FALSE mechanism: javac has NO unused-import -Xlint category, CI runs only `mvn install` (no checkstyle/spotbugs), and the R1 build was clean (0 warnings) WITH the import present. javac does not process Javadoc {@link}, so failOnWarning cannot trip on it. Applied the clean fix regardless: removed the Javadoc-only import and fully-qualified {@link ...Propagation#REQUIRES_NEW} — no dead-in-bytecode import remains and any future checkstyle UnusedImports passes. (2) MINOR A-1 OutboxPublisher.java:184 — deprecated byte[] overload lacked the payload.length==0 guard the Function overload (line 296) already has. Added the guard (IllegalArgumentException "payload must not be zero-length") + @throws Javadoc + @param "not empty". A zero-byte event is never a valid outbox payload; the guard fires before the TX-check, co-located with the other arg validation. (3) INFO B-2 OutboxPublisher.java:176 — @Deprecated(since="1.1") mismatched artifact coordinate 1.1.0-SNAPSHOT. Changed since to "1.1.0" and aligned the @deprecated prose ("since v1.1.0"). Fix: all three confined to OutboxPublisher.java. No public API-shape change — the new overload signature is untouched; the byte[] overload only gains a fail-fast guard for an input that was never valid (no existing test or caller passes an empty payload). Verification: - mvn -B -pl outbox-publisher -am clean verify → BUILD SUCCESS - failOnWarning=true + -Xlint:all → 0 compiler [WARNING] lines - OutboxPublisherTest 27, unit suite 81, OutboxPublisherIT 4 — 0 failuresR2 test changes (findings A-1 + A-2): (1) A-1 T13 publishByteArrayEmptyPayloadThrowsIae — proves the new byte[] empty-guard fires (IAE containing "zero-length", no row saved). Run inside a TX so the assertion isolates the empty-guard from the outside-TX guard. Symmetric with T7 (Function overload). OutboxPublisherTest 26 → 27. (2) A-2 OutboxPublisherIT.publishFunctionalLockstepEventIdMatchesPayloadField — removed the tautological second findById(PK(aggregateId, returned)) round-trip + the redundant payload==rowEventId assertion: findById on the compound PK can only return a row whose eventId == returned, so it isolated no distinct failure mode. Folded a meaningful row.getSentAt() != null assertion into the await() block (consistent with the happyPath IT). The lockstep proof (payload event_id == publish() return) is preserved; payload event_id == row event_id holds transitively and is documented in-test. Verification: - mvn -B -pl outbox-publisher -am clean verify → BUILD SUCCESS - OutboxPublisherTest 27, OutboxPublisherIT 4 — 0 failuresShow previous round
hib-pr-reviewer review — PR #6 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
20ee9be77b2f, basemain, triggersynchronizeTL;DR: NO_NEW_FINDINGS — No new findings this round.
Summary
[quorum-converged] A=0 = B=0. ## Round 2 review — PR #6 Function<UUID,byte[]> overload
All three round-1 findings are resolved:
Propagationimport): removed; the@linkat line 246 uses the fully-qualified name, no import needed. ✅sentAtcheck absent / redundantfindById):assertThat(row.getSentAt()).isNotNull()added at IT:224; explanation comment at IT:238-244 correctly argues why a secondfindByIdis unnecessary. ✅@Deprecated(since="1.1")version mismatch): annotation now reads@Deprecated(since = "1.1.0", forRemoval = false). ✅One new minor finding in the added unit-test file:
tripFastConfig()atOutboxPublisherTest.java:232is defined as astatichelper with the comment "Used by the integration test to verify the 11th publish short-circuits", but no integration test (nor any other test file in the package) calls this method. The@SuppressWarnings("unused")annotation acknowledges its dead state. The comment is a stale forward-reference that will mislead future maintainers searching for a non-existent IT.CI status (head
20ee9be77b2f)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 2 • 0 findings • 2026-05-28T06:28:41.336Z → 2026-05-28T06:33:15.192Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]
R3 finding (kept=1; R2 prose flag, dropped from the structured table due to the reviewer's bookkeeping-fallback path — addressed regardless per rule 18/62): (1) MINOR OutboxPublisherTest.java:232 — tripFastConfig() was a static helper annotated @SuppressWarnings("unused") with a comment claiming it is "Used by the integration test to verify the 11th publish short-circuits". Grep confirms ZERO callers (only its own definition matched). The referenced IT was never written; the suppression masked the dead state and the comment was a stale forward-reference that would mislead future maintainers. Removed the method + its Javadoc. The circuit-open behaviour it gestured at is already covered by hotPublishOnCircuitOpenSkipsKafka (manual OPEN → Kafka skipped) and asyncCallbackErrorFeedsCircuitBreakerOnError (async failures feed the breaker). CircuitBreakerConfig (line 62, 245) + Duration imports remain used. Pre-existing — predates this PR's keystone changes. Fixed per rule 18 (encountered issue, <15 min) + rule 62 #7 (dead code / unnecessary @SuppressWarnings in a file this PR touches). Verification: - mvn -B -pl outbox-publisher -am clean install → BUILD SUCCESS - failOnWarning=true + -Xlint:all → 0 compiler [WARNING] lines - OutboxPublisherTest 27, unit suite 81, OutboxPublisherIT 4 — 0 failuresShow previous round
hib-pr-reviewer review — PR #6 (affinity-intelligence-rework/im2be-platform-libs)
Round 3 — head
873d4690c90b, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 unique-to-B minor finding (FQN Pattern usage, verified at line 231); no blocking or major issues; all prior-round findings confirmed fixed.
Summary
Arbiter — Round 3 reconciliation
Agreed findings: None (A=0, B=1).
Unique-to-B finding (line 231 FQN
java.util.regex.Pattern): Verified viaReadonOutboxPublisherIT.javalines 1–35 (import block) and lines 220–244 (usage site). The import block contains noimport java.util.regex.Pattern;— the class is referenced exclusively via its fully-qualified name on line 231. The finding is factually accurate. Severityminoris correct: the code compiles and runs correctly, but the inline FQN is non-idiomatic and makes the already-dense regex chain harder to scan.Reviewer A raised no new findings after a full read of all three files; nothing in A's narrative contradicts or disputes the B finding.
Kept: 1 unique-to-B finding, verified.
Dropped: 0.
Blast Radius
Changes are confined to one module (
outbox-publisher): a newFunction<UUID,byte[]>overload inOutboxPublisher.java, extended unit tests, and an additional IT test. The five pom.xml version bumps are mechanical and affect sibling modules only as consumers of the parent version string. No shared protocol contracts or migration files are touched.BLAST_SCORE: 2/10
CI status (head
873d4690c90b)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR] FQN
java.util.regex.Patternused without import — non-idiomatic Javaoutbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPublisherIT.java:231
Line 231 calls
java.util.regex.Pattern.compile(...)via its fully-qualified class name. The import block (lines 1–35) has noimport java.util.regex.Pattern;, confirming this is intentional FQN usage rather than a missing import that was silently resolved.The code is functionally correct, but FQN notation inside method bodies is non-idiomatic in Java test code and makes the already-dense regex chain harder to read at a glance.
Fix: add
import java.util.regex.Pattern;alongside the otherjava.util.*imports and replace line 231 with:Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-05-28T06:38:47.454Z → 2026-05-28T06:39:16.683Z • posted-as: pr-reviewer-bot
R4 finding (kept=1; CONDITIONAL_APPROVE, unique-to-B): (1) MINOR OutboxPublisherIT.java:231 — the lockstep IT referenced java.util.regex.Pattern via its fully-qualified name inline in the method body. Non-idiomatic and inconsistent with the file's other java.util.* / java.time.* imports (rule 10 — match existing conventions). Added `import java.util.regex.Pattern;` and use the short name in the matcher chain. Functionally identical (regex unchanged). This corrects my own R1 lockstep-test code. Verification: - mvn -B -pl outbox-publisher -am clean verify → BUILD SUCCESS - OutboxPublisherTest 27, unit suite 81, OutboxPublisherIT 4 — 0 failures - failOnWarning=true + -Xlint:all → 0 compiler [WARNING] lineshib-pr-reviewer review — PR #6 (affinity-intelligence-rework/im2be-platform-libs)
Round 4 — head
98a8ea482801, basemain, triggersynchronizeTL;DR: NO_NEW_FINDINGS — No new findings this round.
Summary
[quorum-converged] A=0 = B=0. Round 4 — All prior findings resolved; no new actionable findings.
Verified the current state of every open finding:
Propagationunused import atOutboxPublisher.java:23TransactionSynchronizationManager)findById+ missingsentAtcheck atOutboxPublisherIT.java:224await()now usesfindById(new OutboxRecord.PK(aggregateId, returned))directly; bothSENTandsentAt != nullare asserted (lines 224–225)@Deprecated(since = "1.1")version mismatch@Deprecated(since = "1.1.0", forRemoval = false)matching the artifact versionjava.util.regex.Patternwithout importimport java.util.regex.Pattern;added at line 13;Pattern.compile(…)used at line 232The 13 new unit tests (T1–T13) for the
Function<UUID, byte[]>overload cover all documented contracts: outside-TX guard fires beforepayloadFn, exactly-once invocation, before-save ordering, exception propagation, null return, empty-array return, UUIDv7 invariant, return-value equality, and defensive clone for both overloads. The end-to-end IT lockstep test correctly uses the uniquemarkersentinel to correlate the Kafka message to the specific publish call without false-positives from prior test records.CI status (head
98a8ea482801)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-28T06:41:46.989Z → 2026-05-28T06:44:56.075Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]