Skip to content

[#4801] Keep the work package's stored token monotonic from the first store of a claim - #4807

Open
schananas wants to merge 1 commit into
mainfrom
bug/4801/seed-last-stored-token-from-claim
Open

[#4801] Keep the work package's stored token monotonic from the first store of a claim#4807
schananas wants to merge 1 commit into
mainfrom
bug/4801/seed-last-stored-token-from-claim

Conversation

@schananas

@schananas schananas commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #4801
Fixes #4836

What changed

Two parts, and the first cannot ship without the second.

1. Seed the work package's stored token from the claim (#4801)

lastStoredToken had no initialiser while lastDeliveredToken and lastConsumedToken were both seeded from builder.initialToken. The monotonicity guard is written lastStoredToken != null && ..., so it was skipped on the first store of every claim cycle, per segment. A progress strategy offering a low position at that moment durably rewound the segment with no warning, and the whole segment was redelivered on the next claim.

2. Make coversWhenUnwrapped null-tolerant (#4836)

WrappedToken.unwrapUpperBound returns null for a null-current ReplayToken and for a half-empty MergedTrackingToken, and the helper dereferenced it. GlobalSequenceTrackingToken.covers(null) returns true, so this hid; GapAwareTrackingToken.covers(null) throws, and the JPA engine's firstToken() returns a GapAwareTrackingToken that Spring Boot wires by default.

TrackingToken unwrappedReference = WrappedToken.unwrapUpperBound(reference);
if (unwrappedReference == null) {
    return true;
}
TrackingToken unwrappedCandidate = WrappedToken.unwrapUpperBound(candidate);
return unwrappedCandidate != null && unwrappedCandidate.covers(unwrappedReference);

A null reference means no raw stream position is recorded, so there is nothing to regress from and accepting the store is behaviour-preserving -- pre-seed, a null lastStoredToken skipped the guard and the store went through anyway. Rejecting would wedge a just-reset segment for ever. The candidate side is guarded too: an unguarded version NPEs when the candidate unwraps to null, and returning false matches the method's own promise for an incomparable token.

There is exactly one production call site, WorkPackage:518, so the reasoning only has to hold there.

The second defect is pre-existing, not introduced here

On pure main the second store of the idle-beat sequence already throws IllegalArgumentException: Incompatible token type provided: null: upkeepIfThresholdIsMet sees Objects.equals(replayToken, null) as false, drives onBatchCommit and persists the null-current ReplayToken, and the guard is skipped because lastStoredToken is null, so the empty token becomes the reference. The seed only moves that empty reference from store #1 to construction; the helper fix closes both.

Tests

With both fixes: 258 testcases, 0 failures. Each half is independently load-bearing: removing the helper fix gives 3 errors (the WorkPackage case and both TrackingTokenUtils cases), removing the seed gives exactly 1 failure, persistProgressIgnoresATokenBehindTheClaimedTokenOnTheFirstStoreOfAClaim.

Across WorkPackageTest (35), TrackingTokenUtilsTest (14), ReplayTokenTest (31), MergedTrackingTokenTest (20), GapAwareTrackingTokenTest (33), CoordinatorTest (22), PooledStreamingEventProcessorTest (70), TokenStoringProgressStrategyTest (5), ReplayTokenWrappingComplexTokenTest (28).

Why CI could not have caught this

Every existing test touching the default replay initial token uses GlobalSequenceTrackingToken, the one type that tolerates covers(null). Nothing in the tree pairs a pooled processor with a GapAwareTrackingToken, so the full 4253-test messaging suite stays green on the broken code. The new WorkPackageTest case uses ReplayToken(GapAwareTrackingToken(5), null) to close that blind spot.

For the reviewer

The guard in storeIfAdvanced and the idle-upkeep gate are otherwise unchanged; they simply now have a non-null reference on the first store after a claim.

A second intended effect: the idle gate Objects.equals(lastConsumedToken, lastStoredToken) never held on a freshly claimed idle segment, contradicting its own Javadoc that an idle segment never drives a strategy with an unchanged position. Seeding makes it hold, so a freshly claimed idle segment stops invoking onBatchCommit until something is consumed. No strategy can be starved: hasPendingWork() routes an idle segment through a commit cycle before the upkeep gate. Claim extension is unaffected.

Left alone deliberately: WrappedToken.unwrapUpperBound is declared non-null in a @NullMarked package but returns null for these shapes. Marking it @Nullable is the honest fix and cascades to every caller in the tree; ReplayToken:129 already null-checks its result, so the precedent for handling it in place exists.

@schananas
schananas requested a review from a team as a code owner July 29, 2026 11:31
@schananas
schananas requested review from hatzlj, hjohn and jangalinski and removed request for a team July 29, 2026 11:31
@schananas schananas self-assigned this Jul 29, 2026
@schananas
schananas marked this pull request as draft July 29, 2026 17:34
… store of a claim

The `lastStoredToken` field was left null by the constructor while `lastDeliveredToken` and
`lastConsumedToken` were seeded from the claim. Since the monotonicity guard is written as
`lastStoredToken != null && ...`, it was skipped on the first store of every claim cycle. A progress
strategy offering a low position at that moment durably rewound the segment with no warning, and the
whole segment was redelivered on the next claim.

The claimed token is what the store last accepted for that segment, so seeding the field from it
makes the guard cover the first store too. It also makes the idle-upkeep gate hold on a freshly
claimed idle segment, which its own Javadoc already promised.

Seeding alone is not enough, because a non-null token can still unwrap to no raw position at all.
The default initial token is a `ReplayToken` with a null current token, and `MergedTrackingToken`
unwraps to null while one half is empty. `coversWhenUnwrapped` passed that null straight into
`covers`, which `GapAwareTrackingToken` -- the token a JPA-backed event store hands out -- rejects
with an `IllegalArgumentException`. The throw lands inside the batch unit of work, so the package
aborts and the re-claimed package meets it again with the token never stored. That hole was already
reachable without the seed, through an idle beat firing before the first event and storing the
null-current `ReplayToken`.

The guard therefore decides on the unwrapped positions it actually has: a reference without a raw
position is nothing to regress from and is covered by anything, while a candidate without one
advances to nothing and covers nothing. Neither is handed to `covers`, as raw token types are not
required to tolerate a null argument.

Fixes #4801
@schananas
schananas force-pushed the bug/4801/seed-last-stored-token-from-claim branch from a695e9b to b8f98f6 Compare July 29, 2026 17:48
@schananas schananas changed the title [#4801] Seed the work package's last stored token from the claimed token [#4801] Keep the work package's stored token monotonic from the first store of a claim Jul 29, 2026
@schananas
schananas marked this pull request as ready for review July 29, 2026 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant