Skip to content

Replication: ServerHandler.abortStart rolls back a concurrently-adopted generation ID on cross-connect abort (GenerationIdTest.testMultiRS flake) #735

Description

@vharseko

Symptom

GenerationIdTest.testMultiRS is still intermittently failing in CI after #725 (re-advertising fix):

org.assertj.core.api.SoftAssertionError:
The following assertion failed:
1) [in replServer3] expected:<[48]L> but was:<[-1]L>
	at org.opends.server.replication.GenerationIdTest.assertGenIdEquals(GenerationIdTest.java:1043)
	at org.opends.server.replication.GenerationIdTest.waitForStableGenerationId(GenerationIdTest.java:1027)
	at org.opends.server.replication.GenerationIdTest.testMultiRS(GenerationIdTest.java:927)

One RS out of three stays at generation ID -1 for the whole 120 s polling window while the other two converge on the DS generation ID. Example run: https://github.com/vharseko/OpenDJ/actions/runs/29246575640/job/86804802875 (ubuntu, JDK 26 — the JDK/OS is irrelevant, it is a timing race).

Timeline reconstructed from the test trace dump (all within the same second)

  1. The test starts RS1(911), RS2(912), RS3(913); DS(901) connects to RS1 with generation ID 48.
  2. RS3's connector thread starts an outgoing connection to RS2. ReplicationServerHandler.connect() unconditionally snapshots the rollback value before changing anything: oldGenerationId = localGenerationId = -1 (ReplicationServerHandler.java:153-154).
  3. Concurrently, RS3's ServerReader (from RS1) receives a TopologyMsg carrying generation ID 48 and legitimately adopts it: receiveTopoInfoFromRS(..., allowResetGenId=true)setGenerationIdIfUnset(48). RS3's domain generation ID is now 48 (confirmed by the subsequent mayResetGenerationId skip RS 912 ... different genId trace, which is only possible with local genId 48).
  4. Simultaneous cross-connect: RS2 has already connected to RS3 in the opposite direction, so RS2's listener rejects RS3's incoming dial (isAlreadyConnectedToRSabortStart, ReplicationServerHandler.java:296) and sends a StopMsg.
  5. RS3's connector thread receives the StopMsgabortStart(null) (ReplicationServerHandler.java:172). In ServerHandler.abortStart (ServerHandler.java:228-231), oldGenerationId (-1) != -100, so it executes changeGenerationId(-1)blindly rolling the domain back to the value it had when the connect started, wiping the concurrently adopted 48 (and clearing the changelog DBs as a side effect).
  6. The topology is stable from then on: RS1's generation ID never changes again, so the Fix intermittent GenerationIdTest.testMultiRS by re-advertising genId on change #725 re-advertise (which only fires on a genId transition) never repeats. RS3 stays at -1 forever → test timeout.

Root cause

The rollback in ServerHandler.abortStart is meant to undo the handler's own generation ID change (the changeGenerationId calls in connect():197-201 and checkGenerationId():482), but it is armed unconditionally at the start of connect() and executes on any handshake abort, without checking whether another thread modified the domain in between. If a concurrent ServerReader from another RS legitimately updates the domain generation ID inside that window, the abort destroys it.

The incoming path is not affected: processStartFromRemote/startFromRemoteRS set the sentinel oldGenerationId = -100 ("nothing changed") before any abort can happen (ReplicationServerHandler.java:91 and :286). Only the outgoing connect() that loses a cross-connect race is vulnerable — which is why exactly one RS gets clobbered.

Reproduction window: the generation ID gossip must land between the start of an RS's outgoing connect and its cross-connect abort — hence the flakiness, and hence why it hits the RS whose outgoing dial loses the race.

Why #725 did not fix it

#725 addressed the "peer missed the advertisement" scenario. Here the advertisement was received and adopted — then destroyed by the local rollback. No further advertisement will ever come because the sender's generation ID never changes again.

Suggested fix

Minimal: in ReplicationServerHandler.connect(), do not pre-arm the rollback — initialize oldGenerationId = -100 instead of snapshotting localGenerationId at line 154. The real old value is already assigned exactly where changeGenerationId is actually called (connect():197-201, checkGenerationId():482).

More robust: give the rollback compare-and-set semantics — in abortStart, only roll back if the domain's current generation ID still equals the value this handler wrote.

The same pattern should be reviewed in DataServerHandler (lines 368/398/604).

Metadata

Metadata

Assignees

Labels

bugconcurrencyThread-safety / race-condition bugsreplicationtestsTest suites: fixing, enabling, un-disabling

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions