From e193d73b6b8d893cebf46d0eb2ee9d3735b690f6 Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Fri, 11 Sep 2026 13:07:27 +0000
Subject: [PATCH] [#967] Keep the bookkeeping of a domain whose base entry is missing out of its configuration entry (#972)
---
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java | 26 +++---
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/StateWithoutBaseEntryTest.java | 140 +++++++++++++++++++++++++++++++++++
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java | 27 ++++--
3 files changed, 173 insertions(+), 20 deletions(-)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
index 56ec859..8daed89 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
@@ -371,6 +371,13 @@
* stops the session as its first act, so what they wait for is a session which is about
* to be stopped again. The wait between the stop and the start is deliberately left
* outside the lock, so the waiting is bounded by a connect rather than by the backoff.
+ * <p>
+ * It comes after the configuration backend's update lock and never before it: a write to
+ * the domain configuration entry holds that lock while it calls
+ * {@link #applyConfigurationChange(ReplicationDomainCfg)}, which takes this one. So
+ * nothing may write a configuration entry while holding this lock - that is why neither
+ * the state {@link #disable()} saves nor the generationId {@link #enable()} stores falls
+ * back to the domain configuration entry when the base entry of the suffix is missing.
*/
private final Object serviceStateLock = new Object();
/**
@@ -4267,6 +4274,17 @@
/**
* Stores the value of the generationId.
+ * <p>
+ * A base entry which is not in the backend leaves the generationId unstored until the
+ * entry appears, and is not an error - it is what a suffix waiting to be initialized by
+ * an import looks like. The generationId used to be stored on the domain configuration
+ * entry instead, and must not be again: that write reaches
+ * {@link #applyConfigurationChange(ReplicationDomainCfg)} with the configuration
+ * backend's update lock held, and so takes {@link #serviceStateLock} in the order
+ * opposite to the one {@link #disable()} takes the two in. The generationId of a suffix
+ * with no entry is a constant which {@code loadGenerationId()} computes again for free,
+ * and a value a former version left on the configuration entry is still read back.
+ *
* @param generationId The value of the generationId.
* @return a ResultCode indicating if the method was successful.
*/
@@ -4276,14 +4294,7 @@
if (result != ResultCode.SUCCESS)
{
generationIdSavedStatus = false;
- if (result == ResultCode.NO_SUCH_OBJECT)
- {
- // If the base entry does not exist, save the generation
- // ID in the config entry
- result = runSaveGenerationId(config.dn(), generationId);
- }
-
- if (result != ResultCode.SUCCESS)
+ if (result != ResultCode.NO_SUCH_OBJECT)
{
logger.error(ERR_UPDATING_GENERATION_ID, getBaseDN(), result.getName());
}
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java
index 6b5bba9..7b7bf70 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PersistentServerState.java
@@ -303,24 +303,26 @@
/**
* Save the current values of this PersistentState object
* in the appropriate entry of the database.
+ * <p>
+ * A base entry which is not in the backend - a suffix waiting to be initialized by an
+ * import - leaves this state unwritten until the entry appears. The state used to be
+ * written to the domain configuration entry instead, and must not be again: that write
+ * goes through the configuration backend, which holds its update lock while it calls
+ * every change listener of the entry back, and the domain is one of them -
+ * {@code LDAPReplicationDomain.applyConfigurationChange()} takes the very lock
+ * {@code disable()} holds while it calls this, so the two orders deadlock.
+ * <p>
+ * Nothing is lost by not writing it. A suffix whose base entry is missing holds no entry
+ * at all, so no change of this replica is in this state, and a change from another one
+ * can not be replayed into it either. The value a former version left on the
+ * configuration entry is still read back by {@link #loadState()}.
*
* @return a boolean indicating if the method was successful.
*/
private boolean updateStateEntry()
{
// Generate a modify operation on the Server State baseDN Entry.
- ResultCode result = runUpdateStateEntry(baseDN);
- if (result == ResultCode.NO_SUCH_OBJECT)
- {
- // The base entry does not exist yet in the database or has been deleted,
- // save the state to the config entry instead.
- SearchResultEntry configEntry = searchConfigEntry();
- if (configEntry != null)
- {
- result = runUpdateStateEntry(configEntry.getName());
- }
- }
- return result == ResultCode.SUCCESS;
+ return runUpdateStateEntry(baseDN) == ResultCode.SUCCESS;
}
/**
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/StateWithoutBaseEntryTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/StateWithoutBaseEntryTest.java
new file mode 100644
index 0000000..dff2b2f
--- /dev/null
+++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/StateWithoutBaseEntryTest.java
@@ -0,0 +1,140 @@
+/*
+ * The contents of this file are subject to the terms of the Common Development and
+ * Distribution License (the License). You may not use this file except in compliance with the
+ * License.
+ *
+ * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the
+ * specific language governing permission and limitations under the License.
+ *
+ * When distributing Covered Software, include this CDDL Header Notice in each file and include
+ * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL
+ * Header, with the fields enclosed by brackets [] replaced by your own identifying
+ * information: "Portions copyright [year] [name of copyright owner]".
+ *
+ * Copyright 2026 3A Systems, LLC.
+ */
+package org.opends.server.replication.plugin;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.opends.messages.ReplicationMessages.ERR_UPDATING_GENERATION_ID;
+import static org.opends.server.TestCaseUtils.TEST_ROOT_DN_STRING;
+
+import org.forgerock.opendj.ldap.DN;
+import org.forgerock.opendj.ldap.ResultCode;
+import org.opends.server.TestCaseUtils;
+import org.opends.server.core.DirectoryServer;
+import org.opends.server.replication.ReplicationTestCase;
+import org.opends.server.replication.common.CSNGenerator;
+import org.opends.server.replication.common.ServerState;
+import org.opends.server.types.Entry;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+/**
+ * Tests that a replication domain whose base entry does not exist keeps its bookkeeping out of
+ * its own configuration entry.
+ * <p>
+ * Writing it there closes a lock cycle: the write goes through the configuration backend, which
+ * holds its update lock while it calls the domain back, and the callback takes the very lock
+ * {@code disable()} holds while it saves the state.
+ */
+@SuppressWarnings("javadoc")
+public class StateWithoutBaseEntryTest extends ReplicationTestCase
+{
+ private static final String SYNC_STATE = "ds-sync-state";
+ private static final String GENERATION_ID = "ds-sync-generation-id";
+ /** Kept clear of the server id of the domain started below, which has a state of its own. */
+ private static final int STATE_SERVER_ID = 42;
+
+ private DN baseDN;
+
+ @Override
+ @BeforeClass(alwaysRun = true)
+ public void setUp() throws Exception
+ {
+ super.setUp();
+
+ baseDN = DN.valueOf(TEST_ROOT_DN_STRING);
+ /*
+ * The suffix has a backend but no base entry, which is what a domain configured over a
+ * backend waiting to be initialized by an import looks like. It is also what sends the
+ * bookkeeping writes to the configuration entry.
+ */
+ TestCaseUtils.initializeTestBackend(false);
+
+ final int replServerPort = TestCaseUtils.findFreePort();
+ final String replServerLdif =
+ "dn: cn=Replication Server, " + SYNCHRO_PLUGIN_DN + "\n"
+ + "objectClass: top\n"
+ + "objectClass: ds-cfg-replication-server\n"
+ + "cn: Replication Server\n"
+ + "ds-cfg-replication-port: " + replServerPort + "\n"
+ + "ds-cfg-replication-db-directory: StateWithoutBaseEntryTest\n"
+ + "ds-cfg-replication-server-id: 105\n";
+ final String synchroServerLdif =
+ "dn: cn=stateWithoutBaseEntryTest, cn=domains, " + SYNCHRO_PLUGIN_DN + "\n"
+ + "objectClass: top\n"
+ + "objectClass: ds-cfg-replication-domain\n"
+ + "cn: stateWithoutBaseEntryTest\n"
+ + "ds-cfg-base-dn: " + baseDN + "\n"
+ + "ds-cfg-replication-server: localhost:" + replServerPort + "\n"
+ + "ds-cfg-server-id: 1\n"
+ + "ds-cfg-receive-status: true\n";
+
+ configureReplication(replServerLdif, synchroServerLdif);
+ }
+
+ @Test
+ public void aStateSaveWithoutABaseEntryWritesNothingToTheConfigurationEntry() throws Exception
+ {
+ final ServerState state = new ServerState();
+ final PersistentServerState persistentState = new PersistentServerState(baseDN, STATE_SERVER_ID, state);
+ assertThat(persistentState.update(new CSNGenerator(STATE_SERVER_ID, state).newCSN())).isTrue();
+
+ persistentState.save();
+
+ assertThat(domainConfigEntry().getAllAttributes(SYNC_STATE))
+ .as("the state of a domain whose base entry is missing reached its configuration entry")
+ .isEmpty();
+ }
+
+ @Test
+ public void aDomainWithoutABaseEntryWritesNoGenerationIdToItsConfigurationEntry() throws Exception
+ {
+ /*
+ * The domain computed and stored its generationId as it was started in setUp(), which is
+ * enough to fail this before the fix whichever order the methods run in. The other store
+ * this class provokes - the disable()/enable() below - has nowhere else to write either.
+ */
+ assertThat(domainConfigEntry().getAllAttributes(GENERATION_ID))
+ .as("the generationId of a domain whose base entry is missing reached its configuration entry")
+ .isEmpty();
+ }
+
+ @Test
+ public void aBaseEntryWhichIsNotThereIsNoFailureToStoreTheGenerationId() throws Exception
+ {
+ final LDAPReplicationDomain domain = MultimasterReplication.findDomain(baseDN, null);
+ assertThat(domain).as("the domain of this test is gone").isNotNull();
+ // The record the error logger writes carries the id of the message rather than its text,
+ // so what is looked for here does not depend on the locale the tests run under.
+ final String failedWrite =
+ "msgID=" + ERR_UPDATING_GENERATION_ID.get(baseDN, ResultCode.NO_SUCH_OBJECT.getName()).ordinal();
+
+ TestCaseUtils.ERROR_TEXT_WRITER.clear();
+ // Stores the generationId again, the way the end of an import does.
+ domain.disable();
+ domain.enable();
+
+ assertThat(TestCaseUtils.ERROR_TEXT_WRITER.getMessages())
+ .as("a base entry which is not there yet was reported as a failure to store the generationId")
+ .noneMatch(record -> record.contains(failedWrite));
+ }
+
+ private Entry domainConfigEntry() throws Exception
+ {
+ final Entry configEntry = DirectoryServer.getEntry(synchroServerEntry.getName());
+ assertThat(configEntry).as("the domain configuration entry is gone").isNotNull();
+ return configEntry;
+ }
+}
--
Gitblit v1.10.0