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

[vharseko]

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.

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() removes only the entry carrying
that CSN - the two-argument ConcurrentMap.remove() the forward guard already uses - so that a
newer announcement of the same replica is left alone. It 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.

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. The five cases #946 and #976 left there
are kept as they were, and three 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: with the announcement made afterwards the shutdown is still held back,
  with it made before it the forward clears it.
• 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 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.

The first new case was watched failing against the ordering this PR changes: moving the
announcement back behind domain.publish() fails it on the assertion that nothing holds the
shutdown back any longer. The second fails without the withdrawal on the same assertion.

Overlaps

• [#918] Record a ReplicaOfflineMsg as sent only when it really was published #946 has landed, and this branch is rebased on it. Its guard around the announcement is gone
  for the reason it predicted; its trace, and its giving up on the message which stayed queued,
  stay. Its test cases are unchanged.
• [#924] Clear the disabled flag before a domain enables its session #971 has landed too. It rewrites enable(), which is elsewhere in LDAPReplicationDomain than
  the announcement this branch moves, so the two merged with nothing to reconcile.
• [#949] Report a ReplicaOfflineMsg the broker refused as not sent #976 has landed, and this branch is rebased onto it (13d57e063c). It is the reason the
  withdrawal exists: with domain.publish() reporting a refusal, an announcement made before the
  publish has something to be withdrawn for. pushCommittedChanges() keeps reporting the CSN of
  the message the broker accepted, so putReplicaOfflineMsg() and the trace behave as [#949] Report a ReplicaOfflineMsg the broker refused as not sent #976 left
  them.
• [#917] Wait for every peer replication server to forward the ReplicaOfflineMsg #947 has landed as well. It needs the announcement to exist before put() records the
  recipients - replicaOfflineMsgDispatched() is a no-op without it. This change makes that
  hold, leaving its awaitedForwarders == null fallback for the replica which picked a remote
  replication server; the other case that fallback named, an announcement recorded after its
  message was relayed, no longer exists, and its comment says so.

[vharseko]

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]

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]

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]

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.