Skip to content

[#950] Announce a ReplicaOfflineMsg before it is published, not after it may have been forwarded - #978

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/950-announce-replica-offline-before-publish
Open

vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/950-announce-replica-offline-before-publish

Conversation

@vharseko

@vharseko vharseko commented Sep 9, 2026

Copy link
Copy Markdown
Member

Fixes #950

The bug

LDAPReplicationDomain.publishReplicaOfflineMsg() recorded the announcement after
pendingChanges.putReplicaOfflineMsg() returned, and that call has already put the message on
the wire: pushCommittedChanges() reaches domain.publish(msg) -> ReplicationBroker.publish()
-> session.publish(msg) before it comes back.

A collocated replication server which forwards the message in that window calls
DSRSShutdownSync.replicaOfflineMsgForwarded() from its ServerWriter, which finds no entry for
the replica and does nothing but notify the monitor. replicaOfflineMsgSent() then installs a
PendingOfflineMsg which nothing will ever remove - the forward it was waiting for has already
happened.

Since #919 that record is the condition of a blocking wait: ReplicationServer.shutdown() calls
awaitReplicaOfflineMsgsForwarded() and, with a peer RS connected, spends the whole
REPLICA_OFFLINE_GRACE_PERIOD on a message which is on the wire and forwarded. Nothing is lost -
the topology has the announcement - it is a bounded delay of the shutdown. Before #919 the stale
record was harmless, and the ordering it depends on has been there since OPENDJ-1453.

#946 has since narrowed which messages are announced - only those which really were published -
but left the ordering alone: the announcement of a published message still follows its publish.

The window is narrow: between the return of session.publish() and the next statement of the
publishing thread, the collocated RS has to read the socket, write the changelog, queue the
message on the peer handler and write it to the peer session. But the cost of losing the race is
precisely the delay the grace period exists to bound.

The change

The announcement moves to the point where the message is published - the ReplicaOfflineMsg
branch of PendingChanges.pushCommittedChanges() - through a ReplicaOfflineAnnouncer the
domain hands to its PendingChanges. It is therefore in place before session.publish() is
reached, and the ConcurrentHashMap it is written to gives the forwarding thread, which reads it
only after reading the socket, the visibility it needs. The forward can no longer precede it.

The announcer the domain hands over is ShutdownSyncAnnouncer, a package-private class of one
domain and one DSRSShutdownSync: announce() is replicaOfflineMsgSent(), withdraw() is
replicaOfflineMsgNotSent(). It is a class rather than an anonymous one so that
PendingChangesTest builds its pending changes with the very announcer the domain uses, and a
swap of the two calls dies there.

Announcing at the publish site, rather than before the whole putReplicaOfflineMsg(), also means
the announcement follows the publication instead of the queueing. A message which a change in
flight holds back (#918) is not announced at all: #946 gives up on such a message rather than
letting it out late, so there is no later publish to announce it at.

That leaves the if (offlineCSN != null) guard #946 put around the announcement nothing to do,
which is what its own description predicted: a message which is not published is not announced.
publishReplicaOfflineMsg() keeps only the trace #946 added, with the wording #976 gave it.

One announcement does have to be withdrawn. Since #976 domain.publish() reports whether the
broker wrote the message, and it refuses one when it has no usable session, when a recovery is
pending, or when it is stopped in between - all after the announcement was made. Such an
announcement is one nobody will ever forward, so pushCommittedChanges() takes it back through
the announcer, and DSRSShutdownSync.replicaOfflineMsgNotSent() withdraws only the entry
carrying that CSN, and wakes the shutdown up as a forward does. This is the shape #950 proposed,
and what "not fixed here: #949" of the earlier revision of this description was waiting for.

What stays announced is what the broker reports as written - not more than that:
Session.publish() returns without writing for a peer which cannot decode the message and once
the session's close is initiated, and the broker reports both as published. That is #976's
contract, unchanged here.

A withdrawal must not take an earlier announcement with it. A replica announces itself offline
on every disableService(), and replicaOfflineMsgSent() replaces the entry of the replica, so
a shutdown whose message went out, followed within the grace period by an enableService()
whose connect fails or raises connectRequiresRecovery, and then by another disableService(),
announces a second message the broker refuses - and withdrawing that one used to empty the slot
the first one was still waiting in. PendingOfflineMsg now keeps the announcement it displaced,
and the withdrawal puts it back: the earlier message, which did go out, keeps its wait. Whatever
is reported about the earlier message while the refused one stands in its place - a forward, a
peer going away - is not seen by it, and the shutdown then waits out what is left of the earlier
message's own grace period; that window is the one refused publish, and the wait it can cost is
bounded by a grace period which is already running.

A trade-off worth naming

The grace period is now counted from just before the publish instead of just after it. Normally
that is microseconds. With the send window closed the broker loops on tryAcquire(500 ms), and a
slow publish eats part of the 5 seconds before the message even leaves. The direction is the safe
one - the wait can only end earlier, never later - and newShutdownDeadline() bounds the whole
shutdown independently.

Tests

PendingChangesTest drives a real DSRSShutdownSync through the production ShutdownSyncAnnouncer.
The five cases #946 and #976 left there are kept as they were, and four are new:

  • theReplicaOfflineMsgIsAnnouncedBeforeItIsPublished reports the forward from inside
    publish(), which is the moment the message reaches the session, so the race is reproduced
    rather than waited for. It asserts from there that the announcement is already in place, and
    afterwards that the forward cleared it.
  • theAnnouncementOfAPublishedMessageStandsUntilItIsForwarded pins that an announcement the
    broker took is not withdrawn: with nobody having forwarded it, the shutdown must wait.
  • theAnnouncementOfAReplicaOfflineMsgTheBrokerRefusedIsWithdrawn checks from inside
    publish() that the announcement is already in place, refuses the message the way a broker
    with no session does, and asserts nothing holds the shutdown back afterwards - so it pins a
    withdrawal, not an announcement which was never made.
  • theReplicaOfflineMsgHeldBackByAChangeInFlightIsNeverAnnounced pins the other half: nothing is
    announced while a change in flight holds the message back, and nothing is announced when that
    change completes either - [#918] Record a ReplicaOfflineMsg as sent only when it really was published #946 gives up on such a message rather than letting it out late.

DSRSShutdownSyncTest grows four cases for the withdrawal: it ends the wait and leaves nothing
behind, it wakes a waiting shutdown up, the withdrawal of an earlier announcement leaves a newer
one of the same replica alone, and the withdrawal of a later announcement gives the earlier one
its wait back - which a forward of the earlier message then ends.

Three mutants were run against the suite, each dying where its name says:

  • the announcement moved back behind domain.publish() -
    theReplicaOfflineMsgIsAnnouncedBeforeItIsPublished and
    theAnnouncementOfAReplicaOfflineMsgTheBrokerRefusedIsWithdrawn, both on "the message must be
    announced before it is published"
    ;
  • the withdrawal run on both arms of if (domain.publish(msg)) -
    theAnnouncementOfAPublishedMessageStandsUntilItIsForwarded;
  • replicaOfflineMsgSent and replicaOfflineMsgNotSent swapped in ShutdownSyncAnnouncer -
    three cases of PendingChangesTest.

theWithdrawalOfALaterMessageGivesTheEarlierOneItsWaitBack was watched failing before the
displaced announcement was kept: "the earlier message went out and nobody has forwarded it yet -
expected false but was true"
.

Overlaps

@vharseko vharseko added bug replication concurrency Thread-safety / race-condition bugs java tests Test suites: fixing, enabling, un-disabling labels Sep 9, 2026
@vharseko
vharseko requested a review from maximthomas September 9, 2026 05:47
@vharseko vharseko removed the java label Sep 9, 2026
@vharseko
vharseko force-pushed the issues/950-announce-replica-offline-before-publish branch from 8175d6e to 848c47c Compare September 9, 2026 10:03
@vharseko

vharseko commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Rebased onto master now that #946 has landed. The conflict both PRs predicted is resolved and the description above is updated to match; no review had been posted yet, so nothing here answers a review comment.

The two changes met in the same three files.

LDAPReplicationDomain.publishReplicaOfflineMsg()#946 wrapped the announcement in if (offlineCSN != null). With the announcement moved to the publish site there is nothing left to guard, so the method keeps only the trace #946 added for the message a change in flight held back:

final CSN offlineCSN = pendingChanges.putReplicaOfflineMsg();
if (offlineCSN == null && logger.isTraceEnabled())
{
  /*
   * The announcement itself is made where the message is published, so nothing has to be
   * reported here: a message which never reached the wire was never announced either.
   */
  logger.trace("Replica " + getServerId() + " of domain baseDN=" + getBaseDN()
      + " could not announce itself offline: a change which is still in flight holds"
      + " the message back, and " + pendingChanges.size() + " change(s) are pending");
}

PendingChanges.java — merged without a conflict, and both halves stand: putReplicaOfflineMsg() still gives up on the message which stayed queued and returns null, while pushCommittedChanges() announces before domain.publish(msg).

PendingChangesTest.java — an add/add conflict, now the union of both files, five cases. The three #946 added are byte-for-byte unchanged; only the newPendingChanges() helper grew the announcer, behind an overload which keeps those three calling it with one argument.

One case needed more than a merge, and it is worth naming. theReplicaOfflineMsgHeldBackByAChangeInFlightIsAnnouncedOnlyWhenItIsPublished asserted that a held-back message is announced once the change in flight lets it out. After #946 there is no such message left to let out - putReplicaOfflineMsg() removes it from the queue rather than leaving it there - so the assertion contradicted master. It is now theReplicaOfflineMsgHeldBackByAChangeInFlightIsNeverAnnounced, and pins both points: nothing announced while the change is in flight, nothing announced when it completes either.

PendingChangesTest, DSRSShutdownSyncTest and ReplicationServerShutdownSyncTest: 25 tests, all green. The regression the first new case exists for is still caught - putting announce() back behind domain.publish() fails it on "the message was forwarded, so nothing must hold the shutdown back any longer".

@vharseko

vharseko commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

For the record, since the run on the pre-rebase head 8175d6e went red:
DependencyTest.addModDelDependencyTest failed there, and it does not look like this change.

The map this branch moves the write of - DSRSShutdownSync.replicaOfflineMsgs - is read only
by ReplicationServer.shutdown() and by the non-DS branch of ServerWriter. That test has a
single replication server and only DS handlers, and its replication server is shut down in the
finally after the assertion, so neither reader runs before it. Ten local runs of
DependencyTest, five with this patch and five without, were green at 5.36-5.48 s against the
30 s budget the failure exhausted.

It looks like #924; the evidence, and a second sighting of the same signature on another
branch, are in
#924 (comment).

@vharseko
vharseko force-pushed the issues/950-announce-replica-offline-before-publish branch from 848c47c to ec70866 Compare September 10, 2026 07:24
@vharseko

Copy link
Copy Markdown
Member Author

Rebased onto master (36d4af9bd7) to pick up the fix for #924. Same single commit, now ec70866ee6,
and git log origin/master..HEAD shows only it. No code of this branch moved: the diff against
master is the same 128 added and 15 removed lines in the same three files.

Only LDAPReplicationDomain.java was touched by both sides, and in different places - #971 rewrote
enable(), this branch moves the announcement out of publishReplicaOfflineMsg() and into
PendingChanges.pushCommittedChanges(). Nothing to reconcile beyond the merge.

This finishes the note above about the red run on the pre-rebase head. That failure now has a name:
it is #924, and the fix for it reached master on 2026-09-09 13:44 UTC, after this branch was cut from
2a7bb9d7ed. What settles it is the logs/access of that job - note that it is
attempt 1 of run
34316279431 (job 102353062700) which holds it, since the job id now serves the cancelled re-run. The
last operation logged there is the MODIFY dn="o=test" which saves the ServerState right after
enable(), at 06:45:06, and then nothing at all for the 30 s the test waits. A delivery which arrives
in that window is given up on and never asked for again, which is exactly what #971 fixes. The ten
local runs reported above were green because the window is narrow, not because the test is unaffected.

Verified on the rebased branch rather than on the old head:

  • opendj-server-legacy test-compiles.
  • PendingChangesTest 5, DSRSShutdownSyncTest 12, ReplicationServerShutdownSyncTest 8 - 25
    tests, no failures.

@vharseko
vharseko force-pushed the issues/950-announce-replica-offline-before-publish branch from ec70866 to 4aeb3b3 Compare September 11, 2026 19:10
@vharseko

Copy link
Copy Markdown
Member Author

Rebased onto master (13d57e063c), where #976 and #947 have landed. Still one commit, now
4aeb3b35d2; git log origin/master..HEAD shows only it. No review had been posted, so nothing
here answers one.

The conflict was with #976, in the ReplicaOfflineMsg branch of pushCommittedChanges(): master
now reads the answer of domain.publish(msg), this branch announces before that call. Taking both
is not enough - an announcement made before a publish the broker then refuses is exactly the stale
record #976 removed, back in a new shape. So the resolution is the one the description of #950
proposed, and what this PR had listed under "not fixed here":

final CSN offlineCSN = msg.getCSN();
replicaOfflineAnnouncer.announce(offlineCSN);
if (domain.publish(msg))
{
  publishedOfflineCSN = offlineCSN;
}
else
{
  // The broker wrote it to no session, so nobody will forward what was announced.
  replicaOfflineAnnouncer.withdraw(offlineCSN);
}

ReplicaOfflineAnnouncer grew withdraw(), and DSRSShutdownSync a matching
replicaOfflineMsgNotSent(): it removes only the entry carrying that CSN - the two-argument
remove() the forward guard already uses - and wakes the shutdown up the way a forward does.
putReplicaOfflineMsg(), its verdict and the trace of publishReplicaOfflineMsg() are as #976 left
them; only the comment above the trace now names the withdrawal.

#947 changed replicaOfflineMsgForwarded() to take the peer id, so the forward the test reports
from inside publish() names one; with no peer recorded the first forward still ends the wait,
which is the fallback #947 kept for exactly this shape. The other case that fallback's comment
named - an announcement recorded after its message was relayed - no longer exists, and the comment
no longer says it does.

Tests, on the rebased head:

  • PendingChangesTest is the union of both sides plus one case, 8 in all. New here:
    theAnnouncementOfAReplicaOfflineMsgTheBrokerRefusedIsWithdrawn checks from inside publish()
    that the announcement is in place, refuses the message, and asserts nothing holds the shutdown
    back afterwards.
  • DSRSShutdownSyncTest grows three cases for the withdrawal: it ends the wait, it wakes a waiting
    shutdown up, and the withdrawal of an earlier announcement leaves a newer one of the same replica
    alone.
  • PendingChangesTest 8, DSRSShutdownSyncTest 25, ReplicationServerShutdownSyncTest 13 - 46
    tests, no failures, -Pprecommit checkstyle included.

Both regressions were watched: with the announcement moved back behind domain.publish(),
theReplicaOfflineMsgIsAnnouncedBeforeItIsPublished fails on "the message was forwarded, so
nothing must hold the shutdown back any longer"
; with the withdraw() call removed,
theAnnouncementOfAReplicaOfflineMsgTheBrokerRefusedIsWithdrawn fails on "the message never
reached the wire, so nothing must hold the shutdown back"
.

The description above is updated to match.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The order is now the right one, and a refusal is no longer silent.

  • Announcing before domain.publish() closes the #950 window by construction: a forward reported from inside the publish can no longer run ahead of the announcement it clears.
  • PendingChanges.ReplicaOfflineAnnouncer keeps PendingChanges off DSRSShutdownSync; the seam is two methods, and the tests build their own announcer through it.
  • replicaOfflineMsgNotSent withdraws with remove(key, value) under a csn.equals guard, so a stale withdrawal cannot take a fresher entry with it.
  • refuseWhilePublishing asserts from inside the publish Answer — the race is reproduced, not waited for.

Blocking

opendj-server-legacy/src/main/java/org/opends/server/replication/service/DSRSShutdownSync.java:115-117, :142-144opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PendingChanges.java:214-224

issue (blocking): A withdrawal empties a slot that announce() has already re-used, so the sent predecessor loses its wait.

replicaOfflineMsgSent is a put(): it replaces the replica's pending entry. On one domain, within the grace period:

  1. disableService()broker.stop() → CSN1 announced and published; the collocated RS still has it queued to the peer RS behind a backlog (the case the forward guard's comment at :205-210 names).
  2. enableService()broker.start() — the connect fails silently (connectionError, ReplicationBroker.java:850-853) or connectRequiresRecovery is raised (LDAPReplicationDomain.java:5356-5363).
  3. Second broker.stop()announce(CSN2) replaces CSN1's entry → publish() returns false → withdraw(CSN2) finds pending.csn.equals(CSN2) and removes the slot.

awaitReplicaOfflineMsgsForwarded() now waits for nothing and CSN1 is never forwarded before the RS goes down — #919's guarantee is gone for the RS downtime. At BASE a refused CSN2 was never announced, so CSN1's wait survived. Producers: restartService() (back-to-back, from readAssuredConfig / readFractionalConfig), the total-update disable() / enable(), followed by shutdown() or another config change.

Suggested shape — a withdrawal puts back what the announcement displaced:

// PendingOfflineMsg
/** The announcement this one displaced and which is still owed its forward; null when there was none. */
private final PendingOfflineMsg displaced;

// replicaOfflineMsgSent
replicaOfflineMsgs
    .computeIfAbsent(baseDN, dn -> new ConcurrentHashMap<>())
    .compute(offlineCSN.getServerId(),
        (id, displaced) -> new PendingOfflineMsg(offlineCSN, System.nanoTime(), displaced));

// replicaOfflineMsgNotSent
if (pending != null && pending.csn.equals(offlineCSN))
{
  if (pending.displaced != null)
  {
    msgs.replace(serverId, pending, pending.displaced);
  }
  else
  {
    msgs.remove(serverId, pending);
  }
}

And the case for the reachable order (DSRSShutdownSyncTest):

/** The shutdown's message went out; the re-enable's was refused: the first one is still owed its forward. */
@Test
public void theWithdrawalOfALaterMessageGivesTheEarlierOneItsWaitBack() throws Exception
{
  final DSRSShutdownSync shutdownSync = new DSRSShutdownSync(LONG_GRACE_PERIOD);
  final CSN sentByTheShutdown = newCSN(SERVER_ID, 1);
  final CSN refusedByTheBroker = newCSN(SERVER_ID, 2);

  shutdownSync.replicaOfflineMsgSent(baseDN1, sentByTheShutdown);
  shutdownSync.replicaOfflineMsgSent(baseDN1, refusedByTheBroker);
  shutdownSync.replicaOfflineMsgNotSent(baseDN1, refusedByTheBroker);

  assertThat(shutdownSync.canShutdown(baseDN1)).isFalse();
  shutdownSync.replicaOfflineMsgForwarded(baseDN1, sentByTheShutdown, RS_ID);
  assertThat(shutdownSync.canShutdown(baseDN1)).isTrue();
}

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/PendingChangesTest.java:90-101, :231-242

issue (blocking): No case pins that the announcement of a published message stands — "withdraw unconditionally" is green 33/33.

Measured: with withdraw(offlineCSN) run on both arms of if (domain.publish(msg)), PendingChangesTest + DSRSShutdownSyncTest pass 33/33. theReplicaOfflineMsgIsAnnouncedBeforeItIsPublished forwards from inside the publish and then asserts only canShutdown == true; the refusal cases end on true as well. The only assertFalse(canShutdown) is inside refuseWhilePublishing (:252), so "announce deleted" dies once and "withdraw always" never. That mutant undoes #919 entirely and passes CI.

/** The announcement of a message the broker took stands until a peer forwards it. */
@Test
public void theAnnouncementOfAPublishedMessageStandsUntilItIsForwarded() throws Exception
{
  final DSRSShutdownSync shutdownSync = new DSRSShutdownSync();
  final PendingChanges pendingChanges = newPendingChanges(domainWhichPublishes(true), shutdownSync);

  pendingChanges.putReplicaOfflineMsg();

  assertFalse(shutdownSync.canShutdown(baseDN),
      "the message went out and nobody has forwarded it yet, so the shutdown must wait for it");
}

And in forwardWhilePublishing, before the forward — then case 1 pins its own name:

if (msg instanceof ReplicaOfflineMsg)
{
  assertFalse(shutdownSync.canShutdown(baseDN), "the message must be announced before it is published");
  shutdownSync.replicaOfflineMsgForwarded(baseDN, msg.getCSN(), RS_ID);
}

Non-blocking

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:806-820

suggestion (non-blocking): The production announcer is exercised by no test — swapping replicaOfflineMsgSent and replicaOfflineMsgNotSent here survives the suite.

Every test builds its own ReplicaOfflineAnnouncer (PendingChangesTest:264-275) or announces by hand. A package-visible ShutdownSyncAnnouncer(DSRSShutdownSync, DN) in place of the anonymous class, plus one case — announcecanShutdown false, withdraw → true — pins the edge.

opendj-server-legacy/src/test/java/org/opends/server/replication/service/DSRSShutdownSyncTest.java:474-513

suggestion (non-blocking): theWaitEndsWhenTheMessageIsWithdrawn pins the wake-up only by elapsed < LONG_GRACE_PERIOD (60 s); a withdrawal that notifies without emptying the slot passes it.

assertThat(shutdownSync.canShutdown(baseDN1)).as("the withdrawn message holds nothing back").isTrue();

opendj-server-legacy/src/test/java/org/opends/server/replication/service/DSRSShutdownSyncTest.java:106-122opendj-server-legacy/src/main/java/org/opends/server/replication/service/DSRSShutdownSync.java:128

todo (non-blocking): Both texts describe an interleaving that cannot happen: announce() and withdraw() run back to back under pushCommittedChanges()'s monitor (PendingChanges.java:175), one announcer per domain, one domain per baseDN per JVM — no "other thread of the domain" announces in between. The reachable second announcement is the one in the blocking issue above; theWithdrawalOfAnEarlierMessageLeavesANewerOneAlone pins the reverse order. Drop "another thread" from the javadoc and "a newer one made in the meantime" from :128, and say what the case does pin: a stale withdrawal is ignored.

opendj-server-legacy/src/main/java/org/opends/server/replication/service/DSRSShutdownSync.java:49

todo (non-blocking): "counted from the moment the message was sent" — with this PR the clock starts at the announcement, before the publish; :393 already says "announced".

PR description

suggestion (non-blocking): "what is announced is what really went out, and nothing else" is stronger than what the broker can report: Session.publish() returns silently for a pre-V8 peer (getBytes() == null, #1014) and after closeInitiated, and publish() reports true. Pre-existing (#976), only noting — "what the broker reports as written" is the claim that holds.

…published, not after it may have been forwarded

The announcement the shutdown of a collocated replication server waits on was recorded
after PendingChanges.putReplicaOfflineMsg() had already put the message on the wire. A
forward which won that race found nothing to clear, and the announcement which followed
it was one nothing would ever remove: ReplicationServer.shutdown() then spent the whole
REPLICA_OFFLINE_GRACE_PERIOD waiting for the forward of a message the topology already
had.

The announcement now sits where the message is published - the ReplicaOfflineMsg branch
of pushCommittedChanges() - so it is in place before session.publish() is reached and the
forward cannot precede it. It goes through ShutdownSyncAnnouncer, the announcer of one
domain and one DSRSShutdownSync, which the domain hands its PendingChanges. Announcing at
the publish site also makes it follow the publication rather than the queueing, which
leaves the guard OpenIdentityPlatform#918 put around the announcement nothing to do: a message a change in
flight holds back is not published, and is therefore not announced either. The trace
which reports such a message stays.

The broker may still refuse the message once it is announced - no usable session, a
recovery pending, or stopped in between - which OpenIdentityPlatform#949 made domain.publish() report. Such
an announcement is one nobody will ever forward, so it is withdrawn through the new
DSRSShutdownSync.replicaOfflineMsgNotSent(), which takes back only the entry carrying
that CSN and wakes the shutdown up as a forward does: what stays announced is what the
broker reports as written.

A withdrawal gives back what the announcement displaced. A replica announces itself
offline on every disableService(), and each announcement replaces the entry of the
replica, so the refused message of a disable which follows a failed re-enable within the
grace period had taken the place of the message the earlier disable did send - and
withdrawing it emptied the slot that message was still waiting in. PendingOfflineMsg now
keeps the announcement it displaced, and the withdrawal puts it back.

New PendingChangesTest cases, all through the production announcer: the forward reported
from within publish() finds the announcement in place and clears it, the announcement of
a message the broker took stands until a peer forwards it, the announcement of a message
the broker refused is withdrawn, and a message held back by a change in flight is never
announced - neither while it waits, nor when the change which held it back completes and
the message is given up on. DSRSShutdownSyncTest covers the withdrawal: it ends the wait
and leaves nothing behind, it wakes a waiting shutdown up, it leaves a newer announcement
of the same replica alone, and it gives an earlier one its wait back.
@vharseko
vharseko force-pushed the issues/950-announce-replica-offline-before-publish branch from 4aeb3b3 to fba06c0 Compare September 14, 2026 14:17
@vharseko

Copy link
Copy Markdown
Member Author

Round 1 addressed in fba06c0ee9; rebased onto master (cebef54070, where #958, #964 and #974 have
landed - none of them touches this code, the merge was clean). Still one commit,
git log origin/master..HEAD shows only it.

Blocking 1 - a withdrawal emptied a slot announce() had re-used. Confirmed as described:
the second disableService() announces CSN2 over CSN1, the broker refuses it on connectionError
or connectRequiresRecovery, and withdraw(CSN2) removed the slot CSN1 was still waiting in - a
regression against BASE, where a refused CSN2 was never announced. Fixed in the shape suggested:
PendingOfflineMsg keeps the announcement it displaced (compute() in replicaOfflineMsgSent),
and replicaOfflineMsgNotSent puts it back with the three-argument replace(), or removes the
entry when there was none. theWithdrawalOfALaterMessageGivesTheEarlierOneItsWaitBack is in, as
proposed, and was watched failing first: "the earlier message went out and nobody has forwarded
it yet - expected false but was true"
.

One residue is named in the javadoc rather than handled: while CSN2 stands in CSN1's place,
whatever is reported about CSN1 - a forward, a peer going away - is not seen by it, and after the
restore the shutdown waits out what is left of CSN1's own grace period. The window is the one
refused publish (immediate on connectionError / connectRequiresRecovery; up to the reconnect
when the session is null), and the cost is bounded by a grace period which is already running.
Walking the displaced chain from the forward guard would close it, but I would rather not add
that for a window this narrow unless you see it differently.

Blocking 2 - nothing pinned that the announcement of a published message stands. Confirmed
by running the mutant: withdraw on both arms of if (domain.publish(msg)) was green 33/33.
theAnnouncementOfAPublishedMessageStandsUntilItIsForwarded is in, and forwardWhilePublishing
asserts from inside the publish that the announcement is already there. Re-run:

  • withdraw on both arms - dies on theAnnouncementOfAPublishedMessageStandsUntilItIsForwarded;
  • announcement moved back behind domain.publish() - theReplicaOfflineMsgIsAnnouncedBeforeItIsPublished
    now dies on its own name, "the message must be announced before it is published", as does
    theAnnouncementOfAReplicaOfflineMsgTheBrokerRefusedIsWithdrawn.

Production announcer untested. ShutdownSyncAnnouncer(DSRSShutdownSync, DN) is a
package-private class now, and PendingChangesTest.newPendingChanges() builds its pending
changes with it rather than with an announcer of its own - so all nine cases go through the
production code, and the swap of replicaOfflineMsgSent / replicaOfflineMsgNotSent dies on
three of them. That seemed better than one dedicated case next to a duplicate.

theWaitEndsWhenTheMessageIsWithdrawn. canShutdown().isTrue() added. One note on the
rationale: a withdrawal which notified without emptying the slot would have failed this case
already, on elapsed < LONG_GRACE_PERIOD - awaitReplicaOfflineMsgsForwarded() re-reads the
remaining grace period after each wake-up and goes back to waiting - but only after the 60 s;
the added assertion makes it immediate and says why.

"Another thread" / "a newer one made in the meantime". Both reworded. The test's javadoc now
says what it pins - a stale withdrawal, of a message the replica has since announced again, is
ignored - and the replicaOfflineMsgNotSent javadoc describes the displaced announcement
instead.

:49 - "announced".

"What really went out, and nothing else". The sentence was in the ReplicaOfflineAnnouncer
javadoc and the commit message rather than the description; all three now say "what the broker
reports as written", and the description names the two silent returns of Session.publish()
as #976's contract, unchanged here.

Tests, on the rebased head, class per JVM: PendingChangesTest 9, DSRSShutdownSyncTest 26,
ReplicationServerShutdownSyncTest 13 - 48, no failures; -Pprecommit reactor and javadoc doclint
green. The description above is updated to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug concurrency Thread-safety / race-condition bugs replication tests Test suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A ReplicaOfflineMsg forwarded before it is recorded leaves a pending announcement nothing will clear, and the shutdown waits out its grace period

2 participants