[#710] Fix replication catch-up re-sending updates with the original assured flag#714
Open
vharseko wants to merge 3 commits into
Open
[#710] Fix replication catch-up re-sending updates with the original assured flag#714vharseko wants to merge 3 commits into
vharseko wants to merge 3 commits into
Conversation
… flag (OpenIdentityPlatform#710) A peer catching up from the changelog DB (following=false, the default right after handshake) re-read updates via fillLateQueue() and received them with the assured flag, mode and safe-data level of the original sender: the NotAssuredUpdateMsg substitution was only applied on the in-memory queue path in ReplicationServerDomain.addUpdate(). Non-eligible peers then replied a spurious AckMsg; for a safe-read cross-group RS whose ack arrived while the ack window was still open, processReceivedAck() auto-unboxed a missing map entry and threw a NullPointerException that closed the peer connection. Normalize updates in ServerHandler.take(), the single point feeding the ServerWriter from both the queue and the catch-up path: keep the assured flag only while the ack window is still open and this server was eligible for an ack when the update was received (ReplicationServerDomain.isExpectedAck), otherwise substitute a NotAssuredUpdateMsg. Harden both processReceivedAck() implementations to ignore acks from servers that are not expected instead of raising a NullPointerException. Add ExpectedAcksInfoTest for the not-expected-ack case and AssuredReplicationServerTest.testCatchUpClearsAssuredFlag, a deterministic regression test that forces the catch-up path.
…n, lock-free ack membership, wider catch-up test coverage - Move the assured-flag decision in ServerHandler.take() after acquirePermitInSendWindow(), so isExpectedAck() reflects the state at send time rather than at dequeue time (the send window may block longer than the assured timeout). - ExpectedAcksInfo.isExpectedServer() now reads an immutable snapshot of the expected server ids taken at construction, instead of reading the lock-protected, value-mutated expectedServersAckStatus map without a lock. - Parameterize testCatchUpClearsAssuredFlag over safe data level 1, safe data level > 1 and safe read; replace fixed sleeps with polling on changelog persistence and on received updates. - Document that take() is the single assured guard complementing the queue-path substitution in addUpdate(), and that the UnsupportedEncodingException branch deliberately fails open to preserve replication delivery.
OpenIdentityPlatform#714) nReceivedUpdates is incremented by the listen thread and polled from the test thread in waitForReceivedUpdates(); mark it (and everyUpdatesAreOk, written before the increment) volatile so the poll observes updates reliably.
maximthomas
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #710.
Problem
When a peer (RS or DS) is not "following" — its
ServerWriteris catching upfrom the changelog DB rather than being served from the in-memory queue —
updates were delivered with the assured flag, mode and safe-data level of
the original sender.
ReplicationServerDomain.put()persists the original wire bytes viapublishUpdateMsg()before building theNotAssuredUpdateMsg, and thesubstitution that strips the assured flag for non-eligible peers only happened
on the in-memory queue path (
addUpdate()).MessageHandler.getNextMessage()in the not-following branch re-reads the update from the changelog
(
fillLateQueue()) andServerWriterpublished it as-is. Every peer starts incatch-up mode right after the handshake (
following = false), so even ajust-connected peer receives its first updates through the changelog path.
Impact
once a safe-data level 1 update is satisfied) received the update with
assuredFlag: trueand replied a spuriousAckMsg, skewing assuredmonitoring counters and silently violating the "assured does not cross group
boundaries" contract.
window is still open (very likely, since catch-up delivery happens ms after
publish),
ExpectedAcksInfo.processReceivedAck()auto-unboxes a missing mapentry (
boolean = map.get(unknownId)) and throws aNullPointerException.It propagates out of
ServerReader's loop, closing the connection to theacking peer — which reconnects, is in catch-up again, and can flap under
steady assured traffic.
AssuredReplicationServerTest.testSafeDataLevelOneandtestSafeReadOneRSComplexfail stochastically on CI (see Enable the remaining slow-group tests in the default build #702, worked aroundthere by waiting until all peers are following).
Fix
Two independent parts:
ServerHandler.take(), the single point feedingServerWriterfrom both thequeue and the catch-up path. The assured flag is kept only while the ack
window is still open and this server was eligible for an ack when the
update was received (
ReplicationServerDomain.isExpectedAck(csn, serverId)consults the live
waitingAcksentry). This also resolves the corner caseraised in the issue: a peer already in
expectedServerskeeps assured;everyone else (different group, connected after
put(), expired window) getsa
NotAssuredUpdateMsg. Assured counter skew is fixed as a side effect.processReceivedAck()in bothSafeReadExpectedAcksInfoandSafeDataExpectedAcksInfoto ignore acks from servers that are not expectedinstead of raising a
NullPointerException.Tests
ExpectedAcksInfoTest(new, precommit) — unit tests that an ack from anot-expected server id is ignored (no NPE) for both safe-read and safe-data.
AssuredReplicationServerTest.testCatchUpClearsAssuredFlag(new, precommit) —deterministic regression test: an assured update is published to the
changelog before a fake RS connects, so it is served through the catch-up
path and must arrive non-assured. Verified this test fails without the fix
(
received update assured flag is wrong ... assuredFlag: true) and passeswith it.
testSafeDataLevelOne/High/SafeReadOneRSComplexprecommit tests staygreen (queue path still keeps assured for eligible peers). 23 test runs, 0
failures.